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
Add collect()
for Single
and Completable
#186
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few small things.
...talk-concurrent-api/src/test/java/io/servicetalk/concurrent/api/completable/CollectTest.java
Show resolved
Hide resolved
* | ||
* @param singles {@link Iterable} of {@link Single}s, results of which are to be collected. | ||
* @param <T> Type of the result of the individual {@link Single}s | ||
* @return A {@link Single} producing a {@link Collection} of all values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any ordering guarantees, or lack of guarantees? I think it would be helpful to mention that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. No there is no ordering guarantees, will add it to the docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. If a user did want to to maintain the order, are there any operators that would allow for that? Presumably maxConcurrency=1
would do it, but it could be useful to have a way of doing it without limiting the concurrency.
Would it make sense, related to another comment on this PR somewhere, to have allOf
that does not guarantee ordering, and collect
which does? (Thought maybe for now that means we just call this allOf
and leave implementing collect
for another time.)
wdyt @NiteshKant, @Scottmitch, @jayv ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are missing a concatMap
and concatMapEager
which preserves order. The latter also runs in parallel. So today, no but tomorrow we may.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we are using consistent methods names with pre-established conventions that don't preserve order, and there are other methods we can add in the future that do preserve order I'm ok with this approach. However preserving order is a common way for folks to reason about the return value.
Single<T> req1Single = ..;
Single<T> req2Single = ..;
Single<T> req3Single = ..;
Collection<T> results = collect(req1Single, req2Single, req3Single).asFuture().get();
// process result[0] and assume it correlates to req1Single
Whether or not this is how we intend for folks to use this API they may use it this way ... we should consider this may be surprising for folks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
servicetalk-concurrent-api/src/test/java/io/servicetalk/concurrent/api/single/CollectTest.java
Outdated
Show resolved
Hide resolved
servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/Completable.java
Show resolved
Hide resolved
* @return A new {@link Completable} that terminates successfully if all the provided {@link Completable}s have | ||
* terminated successfully or any one of them has terminated with a failure. | ||
*/ | ||
public static Completable collect(int maxConcurrency, Completable... completables) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is not ideal to have to switch up the argument order relative to the Iterable
variant to accommodate the var-args ... should we make the ordering consistent? However maybe in some cases this helps disambiguate and avoid casts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason for switching the argument order is the introduction of var-args. We have the follwoing methods (of interest here):
collect(Iterable<Completable> completables)
collect(Iterable<Completable> completables, int maxConcurrency)
collect(int maxConcurrency, Completable... completables)
We can have the argument order consistent between the maxConcurrency
variants by having the maxConcurrency
argument first but then the order deviates from the "overloaded method should have new arguments at the end" requirement when looked besides the non maxConcurrency
variant => collect(Iterable<Completable> completables)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inconsistency for var args works for me
servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/Completable.java
Outdated
Show resolved
Hide resolved
servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/Completable.java
Show resolved
Hide resolved
* @return A new {@link Completable} that terminates successfully if all the provided {@link Completable}s have | ||
* terminated successfully or any one of them has terminated with a failure. | ||
*/ | ||
public static Completable collect(Completable... completables) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like some libraries use collect
for something similar to what we have as reduce
and CompletableFuture
has a method allOf
for roughy the same purpose as what we are calling collect
here. Would allOf
be a better name, or is there some more "standard" operator name for this purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CompletableFuture#allOf
is a bit interesting as the returned CompletableFuture
is a CompletableFuture<Void>
.
public static CompletableFuture<Void> allOf(CompletableFuture<?>... cfs) {
I guess that is the reason a name like collect()
makes less sense there.
It is true that collect()
is synonymous to reduce()
in other libraries but none of them provide a static collect like this too 😄
If you squint on this method enough it is a flattened-reduce! I feel collect()
is more intuitive as a name and more representative of what it is doing.
servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/Single.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM, not strong on this but we may want to consider clarifying the similarities with Completable.collect|merge
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few comments then lgtm
servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/Completable.java
Outdated
Show resolved
Hide resolved
servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/Single.java
Outdated
Show resolved
Hide resolved
__Motivation__ Today, in order to asynchronously collect a known set of `Single`/`Completable`, one has to use `Publisher`. This creates friction as many users are not comfortable dealing with streaming APIs. We can add a shorthand for `collect()` to help such users. __Modification__ Added static `collect()` variants for `Single` and `Completable` Also added `Publisher#flatMapCompletable()` as that is required for `Completable#collect()` __Result__ Less friction for scalar API users.
Motivation
Today, in order to asynchronously collect a known set of
Single
/Completable
, one has to usePublisher
.This creates friction as many users are not comfortable dealing with streaming APIs. We can add a shorthand for
collect()
to help such users.Modification
Added static
collect()
variants forSingle
andCompletable
Also added
Publisher#flatMapCompletable()
as that is required forCompletable#collect()
Result
Less friction for scalar API users.