-
Notifications
You must be signed in to change notification settings - Fork 7.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GroupBy/GroupByUntil Changes #1727
Merged
benjchristensen
merged 2 commits into
ReactiveX:1.x
from
benjchristensen:groupByWithBackpressure
Oct 6, 2014
Merged
GroupBy/GroupByUntil Changes #1727
benjchristensen
merged 2 commits into
ReactiveX:1.x
from
benjchristensen:groupByWithBackpressure
Oct 6, 2014
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This collapses groupByUntil and groupBy into a single groupBy operator. The new implementation has 2 major changes: 1) It supports reactive pull backpressure. 2) Child GroupedObservables can be unsubscribed and they will be cleaned up and then new instances for the same key can be emitted, like groupByUntil, except that now instead of passing in a special durationSelector function, the child can be composed using take/takeUntil/etc to cause an unsubscribe. If the previous non-obvious groupBy behavior is wanted, then instead of unsubscribing, it can be filtered to ignore all further data, which is what the old groupBy used to do when a child was unsubscribed.
// odd/even into 2 lists
Observable.range(1, 100)
.groupBy(n -> n % 2 == 0)
.flatMap(g -> {
return g.toList();
}).forEach(System.out::println);
|
This one unsubscribes each group after 10 but then emits a new group with the same odd/even keys as new values come in: // odd/even into lists of 10
Observable.range(1, 100)
.groupBy(n -> n % 2 == 0)
.flatMap(g -> {
return g.take(10).toList();
}).forEach(System.out::println);
|
//odd/even into lists of 20 but only take the first 2 groups
Observable.range(1, 100)
.groupBy(n -> n % 2 == 0)
.flatMap(g -> {
return g.take(20).toList();
}).take(2).forEach(System.out::println);
|
//odd/even into 2 lists with numbers less than 30
Observable.range(1, 100)
.groupBy(n -> n % 2 == 0)
.flatMap(g -> {
return g.takeWhile(i -> i < 30).toList();
}).filter(l -> !l.isEmpty()).forEach(System.out::println);
|
Observable.from(Arrays.asList("a", "b", "c", "a", "b", "c", "a", "b", "c", "a", "b", "c", "a", "b", "c", "a", "b", "c"))
.groupBy(n -> n)
.flatMap(g -> {
return g.take(3).reduce((s, s2) -> s + s2);
}).forEach(System.out::println);
|
Looks good to me, the semantics are more clearer now. |
Thanks Neeraj for the review. I also confirmed the signature and semantic changes with @headinthebox so am proceeding with this. |
benjchristensen
added a commit
that referenced
this pull request
Oct 6, 2014
Proposed groupBy/groupByUntil Changes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a proposed change to
groupBy
andgroupByUntil
that does the following:groupByUntil
and rolls that functionality intogroupBy
groupBy
GroupedObservable
s can now be unsubscribed and they will be cleaned up and then new instances for the same key can be emitted, likegroupByUntil
worked, except that now instead of passing in a special durationSelector function, the child can be composed usingtake
/takeUntil
/etc to cause an unsubscribe.If the previous non-obvious groupBy behavior is wanted, then instead of unsubscribing, it can be filtered to ignore all further data, which is what the old groupBy used to do when a child was unsubscribed.
The reason for these changes are:
groupByUntil
was very difficult with its existing signature as the duration selector function effectively required aGroupedObservable
being aPublishSubject
which meant multicasting. In this specific case it may have been possible to do backpressure with the multicasting, but it would be difficult and non-obvious as generally multicasting means the stream is "hot" and reactive pull backpressure can't be applied.groupBy
almost always confused people as to what would happen when they unsubscribed a child usingtake
ortakeUntil
. It always surprised people that it meant all further data would be dropped but the key and group would not be garbage collected. Almost always on an infinite stream people determined they neededgroupByUntil
, but only after stumbling around. After speaking with @headinthebox about this briefly we had enough agreement to submit this proposal and discuss further.Now
groupBy
can behave by default as people expect withunsubscribe
and support infinite streams, garbage collection, etc as well as backpressure.I will provide usage examples below.