yihua commented on code in PR #18456:
URL: https://github.com/apache/hudi/pull/18456#discussion_r3035020446
##########
hudi-common/src/main/java/org/apache/hudi/common/util/FutureUtils.java:
##########
@@ -42,24 +44,37 @@ public class FutureUtils {
*/
public static <T> CompletableFuture<List<T>>
allOf(List<CompletableFuture<T>> futures) {
CompletableFuture<Void> union =
CompletableFuture.allOf(futures.toArray(new CompletableFuture[0]));
+ AtomicReference<Throwable> firstFailure = new AtomicReference<>();
futures.forEach(future -> {
// NOTE: We add a callback to every future, to cancel all the other not
yet completed futures,
// which will be providing for an early termination semantic:
whenever any of the futures
// fail other futures will be cancelled and the exception will be
returned as a result
future.whenComplete((ignored, throwable) -> {
if (throwable != null) {
+ firstFailure.compareAndSet(null, throwable);
+ // Note that {@link CompletableFuture#cancel} does not interrupt the
other underlying tasks;
+ // it only marks their futures as cancelled. The tasks will still
run to completion.
futures.forEach(f -> f.cancel(true));
- union.completeExceptionally(throwable);
Review Comment:
🤖 Agreed, the current approach is clearer. Simply swapping the two lines
would fix the immediate race, but it would still rely on the subtle ordering
guarantee that `completeExceptionally` happens-before the cancelled futures'
callbacks propagate to the `allOf` relay — which is technically true but easy
to break if someone later refactors this code. The `AtomicReference` + `handle`
approach makes the invariant explicit and self-documenting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
