1.x: Rename cache(int) to cacheWithInitialCapacity(int) #3498

loganj · 2015-11-05T23:02:57Z

The parameter is a capacity hint, but more frequently confused with a
buffer size like replay(int) than it is correctly understood. It also
offers no guarantees, only the weak hope of optimization.

The parameter is a capacity hint, but more frequently confused with a buffer size like replay(int) than it is correctly understood. It also offers no guarantees, only the weak hope of optimization.

This change renames the method, deprecating the old name. It also adds javadoc calling out that the parameter is not a bound and referencing replay(int).autoConnect() as a way to achieve that behavior.

akarnokd · 2015-11-05T23:07:52Z

Hi. It is part of the public API and can't be just removed. In addition, why did you modify the underlying internal class?

loganj · 2015-11-05T23:10:02Z

How about @Deprecated?

I modified the internal class because the only use of that constructor parameter was in the removed operator. If you'd prefer to deprecate, of course I'll revert that modification.

akarnokd · 2015-11-05T23:13:29Z

We are kind of set for 1.1 in terms of API visibility. How about updating the javadoc and list alternatives?

loganj · 2015-11-05T23:16:29Z

I've pointed developers directly at that javadoc, and they've still misunderstood the parameter. And that's best case! Any developer who just looks at the signature, which is most of them, misunderstands the parameter. Everyone assumes it's a bound on the buffer size, and it's so obvious that why would you read the javadoc?

It'd be great to get this resolved in the 1.x series.

artem-zinnatullin · 2015-11-05T23:59:20Z

Huge +1 to @Deprecated + simlink like cacheWithInitialBufferSize/etc, cache(int) is very, very misleading (every member of our team had problems with understanding that int is not a size of the cache's buffer).

loganj · 2015-11-06T14:09:16Z

Updated with rename, deprecation, and javadoc.

artem-zinnatullin · 2015-11-06T14:24:30Z

src/main/java/rx/Observable.java

@@ -3666,13 +3674,16 @@ public final Boolean call(T first, T second) {
     *  <dt><b>Scheduler:</b></dt>
     *  <dd>{@code cache} does not operate by default on a particular {@link Scheduler}.</dd>
     * </dl>
+     * <p>
+     * <em>Note:</em> The capacity hint is not an upper bound on cache size. For that, consider
+     * {@link #replay(int)} in combination with {@link ConnectableObservable#autoConnect()} or similar.


artem-zinnatullin · 2015-11-06T15:40:50Z

👍

akarnokd · 2015-11-06T17:11:11Z

src/main/java/rx/Observable.java

     * 
     * @param capacityHint hint for number of items to cache (for optimizing underlying data structure)
     * @return an Observable that, when first subscribed to, caches all of its items and notifications for the
     *         benefit of subsequent subscribers
     * @see <a href="http://reactivex.io/documentation/operators/replay.html">ReactiveX operators documentation: Replay</a>
     */
-    public final Observable<T> cache(int capacityHint) {
+    public final Observable<T> cacheWithCapacityHint(int capacityHint) {


Looks good but can't decide if this alias requires Beta or Experimental or could stay plain.

My inclination would be to leave it plain, since we're deprecating the original name.

It'd be great to see this rename carried into 2.x, or even see this form of the operator dropped altogether.

akarnokd · 2015-11-07T06:10:11Z

You can post a PR against 2.x where you can simply rename and no need for deprecation.

akarnokd · 2015-11-07T18:12:56Z

@stealthcode, @abersnaze what's the situation among Netflix devs regarding this?

artem-zinnatullin · 2015-11-18T06:54:18Z

ping Netflix team

stealthcode · 2015-11-18T21:26:50Z

From what I can tell, this is a change to an existing public facing method name and that's a no-no for 1.x. @abersnaze is committing a change to the contributors.md that explains what changes are allowed in patch release, minor releases, and major releases. Method signatures of non-beta and non-experimental methods are only accepted in major version releases.

You could mark both as deprecated and add a new method cacheWithCapacityHint(int).

artem-zinnatullin · 2015-11-18T21:30:48Z

Well, that's what @loganj did in his PR :) No breaking changes, deprecation
and new method.

On Thu, Nov 19, 2015, 00:27 Aaron Tull [email protected] wrote:

From what I can tell, this is a change to an existing public facing method
name and that's a no-no for 1.x. @abersnaze https://github.com/abersnaze
is committing a change to the contributors.md that explains what changes
are allowed in patch release, minor releases, and major releases. Method
signatures of non-beta and non-experimental methods are only accepted in
major version releases.

You could mark both as deprecated and add a new method
cacheWithCapacityHint(int).

—
Reply to this email directly or view it on GitHub
#3498 (comment).

@artem_zin

stealthcode · 2015-11-18T21:48:36Z

Oh, I see. I misread the pull request.

stealthcode · 2015-11-18T22:06:26Z

I don't see the source of the confusion. java.util.ArrayList follows a similar convention in the constructor public ArrayList(int initialCapacity) and that is sufficient for me to understand it's usage. The array backed data type can be reallocated but it's better to tune it with an appropriate initial value.

I suggest that we leave it alone in 1.x and rename it to cacheUnbounded(int initialCapacity) in 2.x. Also we should improve the javadocs to indicate the intended usage since there still seems to be confusion around this.

JakeWharton · 2015-11-19T02:36:11Z

A list is a data structure designed to grow. When you see cache(4) you do not think of a list, but of a fixed size cache, as most will be bounded by some supplied value.

Additionally, an initial size is not required to create an array list and it's a constructor parameter. Not only do you have to opt-in to using this non-default constructor overload, you would have to look at the documentation for any idea what the value is for. This is very different in that the size is required and it's a named method which should describe behavior but is misleading because of prior assumptions around the word "cache".

stealthcode · 2015-11-19T20:17:02Z

@JakeWharton the cache operator does not use a fixed size cache. I think that you are saying that the operator name should be enough to to convey the semantic meaning of an unbounded growing cache. I think that by renaming it to cacheUnbounded(int initialCapacity) that the usage will be made more clear and it would be discoverable by users who know to look for the cache operator.

If the term "cache" is not sufficient to indicate the contract then we should probably consider changing the name to one that does. What would be a better term for an operator that holds an ordered list of all events emitted from an observable to be replayed to all subscribers after the first?

loganj · 2015-11-19T20:20:53Z

@stealthcode Hence the name cacheWithInitialCapacity, introduced in this PR. That name implies it's unbounded and describes the integer parameter.

loganj · 2015-11-19T20:27:59Z

To clarify: I don't really care what it's called in 2.x, as long as it's reasonably unambiguous (i.e., not cache(int)).

abersnaze · 2015-11-20T21:19:47Z

@JakeWharton I think @stealthcode was agreeing that a new method name would make it clearer but not that cacheWithInitialCapacity or that it should be done on the 1.x branch.

I'm not very interested in spending a lot of energy bike shedding over the name so can we just have a vote for we want to do: (vote for as many options as you want in the order or preference)

A) do nothing
B) deprecate cache(int) and make cacheWithInitialCapacity(int) in 1.x
C) deprecate cache(int) and make cacheUnbounded(int) in 1.x
D) remove cache(int) and make cacheWithInitialCapacity(int) in 2.x
E) remove cache(int) and make cacheUnbounded(int) in 2.x

My vote is A, E, C, D, B.

@loganj @stealthcode @akarnokd @JakeWharton @artem-zinnatullin. We'll tabulate the votes on Monday.

akarnokd · 2015-11-20T21:25:57Z

My vote is A, E

stealthcode · 2015-11-20T21:39:11Z

I vote E, A, C

iNoles · 2015-11-20T21:44:44Z

I vote B, D, A

artem-zinnatullin · 2015-11-20T21:46:05Z

B.

For 2.x I'd prefer to get rid of cache or replay and leave only one
operator with different settings without autoconnection by default:
cacheWithLimit/replayWithLimit, cacheUnbounded /replayUnbounded, etc.

On Sat, Nov 21, 2015, 00:26 David Karnok [email protected] wrote:

My vote is A, E

—
Reply to this email directly or view it on GitHub
#3498 (comment).

@artem_zin

abersnaze · 2015-11-20T22:10:08Z

I would like @JakeWharton votes but so far the A is the winner.

loganj · 2015-11-20T22:48:34Z

B, C, D, E. presuming a change in 1.x means also making the change in 2.x.

We have empirical evidence that the name and parameter are confusing to users. I can't imagine why we'd ignore that evidence based on our own lack of confusion.

I also think it's funny that this balloting gives a single "do nothing" equal weight to four "do something" options. By my count, "do something" is winning even before my vote.

dlew · 2015-11-20T22:58:50Z

B, C myself.

I think it's just confusing and have been bitten by cache(int) before.

yogurtearl · 2015-11-21T00:16:01Z

I vote B

JakeWharton · 2015-11-21T01:17:01Z

E&C then D&B then A

On Fri, Nov 20, 2015, 7:16 PM Michael Bailey [email protected]
wrote:

I vote B

—
Reply to this email directly or view it on GitHub
#3498 (comment).

stealthcode · 2015-11-21T01:28:11Z

I think that E&C equate to to just C... (assuming that all api changes made in 1.x are carried over).

abersnaze · 2015-11-24T00:19:23Z

B won and I'm fine with that. @loganj could do the honors of changing this PR to match. Just to be clear that means that Observable will have the methods, cache(), @Deprecated cache(int) and cacheWithInitialCapacity(int) rebased to one commit off of the 1.x branch.

artem-zinnatullin · 2015-12-09T01:45:16Z

Ping @loganj, let's do it!

The parameter is a capacity hint, but more frequently confused with a buffer size like replay(int) than it is correctly understood. It also offers no guarantees, only the weak hope of optimization. This change renames the method, deprecating the old name. It also adds javadoc calling out that the parameter is not a bound and referencing replay(int).autoConnect() as a way to achieve that behavior.

loganj · 2015-12-10T12:42:44Z

Updated to specified naming. Apologies for the delay, was traveling on vacation.

akarnokd · 2015-12-10T12:45:26Z

👍

stealthcode · 2015-12-10T18:55:25Z

👍

1.x: Rename cache(int) to cacheWithInitialCapacity(int)

loganj force-pushed the remove_capacity_hint branch from a67ae75 to f29d7e9 Compare November 6, 2015 14:06

loganj changed the title ~~Remove confusing cache(int) operator~~ Rename cache(int) to cacheWithCapacityHint(int) Nov 6, 2015

artem-zinnatullin reviewed Nov 6, 2015
View reviewed changes

loganj force-pushed the remove_capacity_hint branch from f29d7e9 to ccbca40 Compare November 6, 2015 14:54

akarnokd reviewed Nov 6, 2015
View reviewed changes

akarnokd added the Discussion label Nov 9, 2015

akarnokd changed the title ~~Rename cache(int) to cacheWithCapacityHint(int)~~ 1.x: Rename cache(int) to cacheWithCapacityHint(int) Nov 12, 2015

DavidMGross mentioned this pull request Dec 1, 2015

document new RxJava cacheWithInitialCapacity() operator ReactiveX/reactivex.github.io#153

Open

loganj force-pushed the remove_capacity_hint branch from ccbca40 to bfe7412 Compare December 10, 2015 12:41

loganj changed the title ~~1.x: Rename cache(int) to cacheWithCapacityHint(int)~~ 1.x: Rename cache(int) to cacheWithInitialCapacity(int) Dec 10, 2015

stealthcode pushed a commit that referenced this pull request Dec 10, 2015

Merge pull request #3498 from loganj/remove_capacity_hint

c5a809d

1.x: Rename cache(int) to cacheWithInitialCapacity(int)

stealthcode merged commit c5a809d into ReactiveX:1.x Dec 10, 2015

akarnokd mentioned this pull request Feb 10, 2016

Release 1.1.1 proposed content #3691

Closed

adipascu mentioned this pull request Feb 15, 2016

concatMapEager and maxConcurrent #3710

Closed

1.x: Rename cache(int) to cacheWithInitialCapacity(int) #3498

1.x: Rename cache(int) to cacheWithInitialCapacity(int) #3498

Conversation

loganj commented Nov 5, 2015

akarnokd commented Nov 5, 2015

loganj commented Nov 5, 2015

akarnokd commented Nov 5, 2015

loganj commented Nov 5, 2015

artem-zinnatullin commented Nov 5, 2015

loganj commented Nov 6, 2015

artem-zinnatullin Nov 6, 2015

Choose a reason for hiding this comment

artem-zinnatullin commented Nov 6, 2015

akarnokd Nov 6, 2015

Choose a reason for hiding this comment

loganj Nov 6, 2015

Choose a reason for hiding this comment

akarnokd commented Nov 7, 2015

akarnokd commented Nov 7, 2015

artem-zinnatullin commented Nov 18, 2015

stealthcode commented Nov 18, 2015

artem-zinnatullin commented Nov 18, 2015

stealthcode commented Nov 18, 2015

stealthcode commented Nov 18, 2015

JakeWharton commented Nov 19, 2015

stealthcode commented Nov 19, 2015

loganj commented Nov 19, 2015

loganj commented Nov 19, 2015

abersnaze commented Nov 20, 2015

akarnokd commented Nov 20, 2015

stealthcode commented Nov 20, 2015

iNoles commented Nov 20, 2015

artem-zinnatullin commented Nov 20, 2015

abersnaze commented Nov 20, 2015

loganj commented Nov 20, 2015

dlew commented Nov 20, 2015

yogurtearl commented Nov 21, 2015

JakeWharton commented Nov 21, 2015

stealthcode commented Nov 21, 2015

abersnaze commented Nov 24, 2015

artem-zinnatullin commented Dec 9, 2015

loganj commented Dec 10, 2015

akarnokd commented Dec 10, 2015

stealthcode commented Dec 10, 2015