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 #67

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 #67 into master will decrease coverage by 1.20%. The diff coverage is 95.08%.

Impacted file tree graph

@@              Coverage Diff              @@
##              master      #67      +/-   ##
=============================================
- Coverage     100.00%   98.80%   -1.21%     
- Complexity        80      105      +25     
=============================================
  Files              7        9       +2     
  Lines            212      250      +38     
  Branches          14       21       +7     
=============================================
+ Hits             212      247      +35     
- Partials           0        3       +3     
Impacted Files Coverage Δ Complexity Δ
...n/java/com/spotify/futures/CompletableFutures.java 97.85% <86.36%> (-2.15%) 53.00 <11.00> (+8.00) :arrow_down:
...main/java/com/spotify/futures/CombinedFutures.java 100.00% <100.00%> (ø) 7.00 <7.00> (ø)
...in/java/com/spotify/futures/ExecutionMetadata.java 100.00% <100.00%> (ø) 11.00 <11.00> (?)
...ava/com/spotify/futures/FailFastWithThrowable.java 100.00% <100.00%> (ø) 6.00 <6.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 25bbd6e...2930f7c. Read the comment docs.

JoseAlavez commented 4 years ago

Will work on the missing coverage :)