1.x: groupBy with evicting map factory fix

[davidmoten]

I'm preparing a PR to fix a bug with the operator groupBy where an evicting map factory is specified.

The problem with the operator extends as far as the method signature. This is what it is now:

<K, R> Observable<GroupedObservable<K, R>> groupBy(
    Func1<? super T, ? extends K> keySelector,
    Func1<? super T, ? extends R> elementSelector, 
    Func1<Action1<K>, Map<K, Object>> evictingMapFactory) 

This is what it should be (the only change is Action1<K> becomes Action1<Object>:

<K, R> Observable<GroupedObservable<K, R>> groupBy(
    Func1<? super T, ? extends K> keySelector,
    Func1<? super T, ? extends R> elementSelector, 
    Func1<Action1<Object>, Map<K, Object>> evictingMapFactory) 

and the javadoc will be changed to indicate that the value from the map should be passed to the Action1 and not the key.

This is a breaking change. How do we want this to happen?

I've got a few options:

• replace the method (breaking change, move to 1.4)
• replace the method (breaking change, move to 2.0, that will be confusing!)
• deprecate the existing method and add groupByV2
• deprecate the existing method and throw a DontCallMeException and add groupByV2
• remove the existing method and add groupByV2

Any ideas/preferences? I'm ready to go with the PR but just need to decide the change in the API.

[akarnokd]

Yeah, too bad it is marked stable in 1.x. Did somebody run into a signature problem?

Deprecation but it should remain operational. Also I'd call the new method groupByEvictable or something like it, no version in method names.

[davidmoten]

Yeah, too bad it is marked stable in 1.x. Did somebody run into a signature problem?

Nope, I ran into a logic problem (that I created) while reviewing the old code for transfer to the new code. In short the operator uses the map to lookup an evicted GroupedObservable by key but if it has been evicted then you can't get the GroupedObservable to complete. I haven't looked closely at this yet and am not sure how the old tests were working but I'll report back on this. The new 2.x method uses a map that is contracted to report evictions by value, not key. I imported the new 2.x tests into 1.x and two failed on 1.x till I applied the 2.x technique.

Deprecation but it should remain operational. Also I'd call the new method groupByEvictable or something like it, no version in method names.

Deal. I'll deprecate and make sure the existing method doesn't do anything nasty (possible memory leak by not completing an evicted GroupedObservable) and I lean towards groupByEvicting for the new method.

[akarnokd]

Closing via #5917.