Implement combine with arbitrary number of stages #61

spkrka · 2019-10-15T07:58:45Z

No description provided.

codecov · 2020-03-12T13:31:11Z

Codecov Report

Merging #61 into master will not change coverage by %.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##              master       #61   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity        69        80   +11     
===========================================
  Files              6         7    +1     
  Lines            191       212   +21     
  Branches           9        14    +5     
===========================================
+ Hits             191       212   +21

Impacted Files	Coverage Δ	Complexity Δ
...main/java/com/spotify/futures/CombinedFutures.java	`100.00% <100.00%> (ø)`	`7.00 <7.00> (?)`
...n/java/com/spotify/futures/CompletableFutures.java	`100.00% <100.00%> (ø)`	`45.00 <4.00> (+4.00)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4e1934...5b78ba1. Read the comment docs.

spkrka · 2020-03-12T13:59:54Z

@mbruggmann @dflemstr - would this be ok to merge?

dflemstr · 2020-03-13T13:58:00Z

README.md

+
+If you want to combine more than six futures of different types, use the other `combine` method.
+Since it supports vararg usage, the function is now the first argument.
+The `Combiner` object that is input to the function can be used to extract values from the input functions.


You mean Combined?

Yes, that's a typo, will fix.

dflemstr · 2020-03-13T13:59:19Z

README.md

@@ -86,6 +86,26 @@ CompletableFutures.combineFutures(f1, f2, f3, f4, (a, b, c, d) -> completedFutur
 CompletableFutures.combineFutures(f1, f2, f3, f4, f5, (a, b, c, d, e) -> completedFuture(a + b + c + d + e));
 ```

+#### Combine an arbitrary number of futures
+
+If you want to combine more than six futures of different types, use the other `combine` method.


I guess one argument that could be made here is that if you need more than 6 futures, then it might be time to refactor the code instead.

Also, is something stopping us from simply adding more overloads? I think I would prefer having overloads with 6-31 parameters before resorting to varargs or similar.

Yes, more than 6 futures may be a code smell in the general case. But if choosing between a generic "Combiner" object and adding 31 parameter methods, I'd prefer the combiner.

dflemstr · 2020-03-13T14:00:13Z

src/main/java/com/spotify/futures/Combined.java

+    }
+  }
+
+  Combined(CompletionStage<?>[] stages) {


Can we use Arrays.asList() and delegate to the other ctor, to reduce code duplication?

We could do that, but the code duplication here is very small I think, and we might introduce some extra overhead by unnecessarily converting to a list.

I can almost guarantee to you that the list will be optimized away through escape analysis in this case, and otherwise it is only an 8 byte cost

dflemstr · 2020-03-13T14:01:41Z

src/main/java/com/spotify/futures/CompletableFutures.java

+   * @since 0.4.0
+   */
+  public static <T> CompletionStage<T> combine(
+          Function<Combined, T> function, CompletionStage<?>... stages) {


Here I feel like we could have one overload delegate to the other via Arrays.asList()

Yes, we could. Should we prefer the code reduction/deduplication over potential performance benefits?

Yes, give me a benchmark if you want to prove me wrong :)

dflemstr · 2020-03-13T14:03:59Z

src/main/java/com/spotify/futures/CompletableFutures.java

+   * @since 0.4.0
+   */
+  public static <T> CompletionStage<T> combineFutures(
+          Function<Combined, CompletionStage<T>> function, CompletionStage<?>... stages) {


I'm confused by the suffix -Futures here, what does it mean?

In the CompletionStage class, this is typically called a compose operation, so maybe combineCompose?

In other cases, this is typically called a flatMap operation, so maybe combineFlatMap? (I don't personally like that one)

Ah, I see that we already have methods called combineFutures that were added without me noticing :) So I guess there is some precedent

Yes, perhaps those other ones were poorly named :(

dflemstr · 2020-03-13T14:04:42Z

src/main/java/com/spotify/futures/CompletableFutures.java

+      all[i] = stages[i].toCompletableFuture();
+    }
+    return CompletableFuture.allOf(all)
+            .thenCompose(ignored -> function.apply(new Combined(stages)));


Since this line is the only difference, maybe we could break out common utility methods for the code above this line in all of the method overloads.

Hm, yes. Perhaps should simply be implemented something like this:

public static <T> CompletionStage<T> combineFutures( Function<Combined, CompletionStage<T>> function, CompletionStage<?>... stages) { return dereference(combine(function, stages)); }

But then perhaps the entire method is not really worth including in the library at all..

dflemstr · 2020-03-13T16:24:11Z

src/main/java/com/spotify/futures/Combined.java

+import java.util.List;
+import java.util.concurrent.CompletionStage;
+
+public class Combined {


Nit: Consider naming this CombinedFutures or something to reduce the likelihood of collisions.

Also nit: Consider declaring this class as final since it is not meant to be extended.

freben approved these changes Mar 12, 2020

View reviewed changes

dflemstr suggested changes Mar 13, 2020

View reviewed changes

spkrka added 2 commits March 13, 2020 16:18

Implement combine with arbitrary number of stages

eee340f

Simplified, removed unnecessary code

702f0dd

spkrka force-pushed the krka/more_combine branch from 8a32829 to 702f0dd Compare March 13, 2020 15:42

dflemstr reviewed Mar 13, 2020

View reviewed changes

Address comments

5b78ba1

spkrka requested a review from dflemstr March 16, 2020 08:27

dflemstr approved these changes Mar 20, 2020

View reviewed changes

dflemstr merged commit 25bbd6e into master Mar 20, 2020

dflemstr deleted the krka/more_combine branch March 20, 2020 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement combine with arbitrary number of stages #61

Implement combine with arbitrary number of stages #61

spkrka commented Oct 15, 2019

codecov bot commented Mar 12, 2020 •

edited

Loading

spkrka commented Mar 12, 2020

dflemstr Mar 13, 2020

spkrka Mar 13, 2020

dflemstr Mar 13, 2020

dflemstr Mar 13, 2020

spkrka Mar 13, 2020

dflemstr Mar 13, 2020

spkrka Mar 13, 2020

dflemstr Mar 13, 2020

dflemstr Mar 13, 2020

spkrka Mar 13, 2020

dflemstr Mar 13, 2020

dflemstr Mar 13, 2020

dflemstr Mar 13, 2020

spkrka Mar 13, 2020

dflemstr Mar 13, 2020

spkrka Mar 13, 2020

spkrka Mar 13, 2020

dflemstr Mar 13, 2020

spkrka Mar 16, 2020

Implement combine with arbitrary number of stages #61

Implement combine with arbitrary number of stages #61

Conversation

spkrka commented Oct 15, 2019

codecov bot commented Mar 12, 2020 • edited Loading

Codecov Report

spkrka commented Mar 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 12, 2020 •

edited

Loading