spotify / completable-futures

Utilities for working with futures in Java 8
Apache License 2.0
393 stars 51 forks source link

Fail fast implementation for joinList #68

Closed JoseAlavez closed 4 years ago

JoseAlavez commented 4 years ago

Hi, we use this library on one of the projects I work on and I noticed an undesirable behavior that gets improved with this PR.

Context:

We use this library in combination with Netty, to consume REST APIs that provide us with single fetch contracts (as for example GET /domain/resource/{id}). In other words, when we require to fetch a batch of resources by id, we create individual asynchronous HTTP calls to the respective APIs and then join the completable futures using .collect(CompletableFutures.joinList()).

When our app is under heavy load and our fetch batch logic leads to individual HTTP calls that grows dramatically, we are seeing execution times that are higher than the timeouts specified for each completable future. This is caused due to the capacity of our executor to perform each HTTP call (and queuing) but also because the current implementation of joinList and allAsList utilizes CompletableFuture#allOf, which will wait until all the states are completed to either continue normally or exceptionally.

Proposal

I took the chance of implementing a extendable fail fast behavior in the joinList and allAsList methods. An interface that can have multiple implementations can be passed as parameter to define how and when the combined completable future needs to fail fast (complete exceptionally) once one or more stages fail. Furthermore, the execution of the incomplete stages can be cancelled when failing fast (understanding that it will cancel further stage chaining and not the current execution of the stage).

Side Notes:

Let me know if this PR brings value to the code base (as we look forward to use it in our project), or if anything here needs to be adapted or if the request would be discarded.

codecov[bot] commented 4 years ago

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff             @@
##              master       #68   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity        84        87    +3     
===========================================
  Files              7         7           
  Lines            222       228    +6     
  Branches          16        17    +1     
===========================================
+ Hits             222       228    +6     
Impacted Files Coverage Δ Complexity Δ
...n/java/com/spotify/futures/CompletableFutures.java 100.00% <100.00%> (ø) 52.00 <6.00> (+3.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 0a318f8...be04285. Read the comment docs.

spkrka commented 4 years ago

Since @mbruggmann and @dflemstr have been the primary developers of this library, I added them as reviewers.

JoseAlavez commented 4 years ago

@mbruggmann @dflemstr @spkrka I have dropped all the excess and left the essential core of what I was after.

I'm still looking forward to cancel the remaining incomplete tasks when failing fast, but I will prefer to do this in another PR and after discussing with you guys whether new method or parameters would be ideal to extend your API.

Sorry for the long discussions and my sloppy approach with the initial PR review. Thanks for the feedback!

spkrka commented 4 years ago

Really nice, short and sweet!

mbruggmann commented 4 years ago

Thanks a lot for your contribution @JoseAlavez 🎉