Re: [PR] [fix][ml] Complete the original future after ledger deletion retries [pulsar]

["Denovo1998 (via GitHub)" <gi...@apache.org>]

Denovo1998 commented on code in PR #26742:
URL: https://github.com/apache/pulsar/pull/26742#discussion_r4230951569


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -3707,7 +3708,7 @@ private void asyncDeleteLedgerWithRetry(CompletableFuture<Void> future, long led
                     future.completeExceptionally(BKException.create(rc));
                     return;
                 }
-                scheduledExecutor.schedule(() -> asyncDeleteLedger(ledgerId, retry - 1),
+                scheduledExecutor.schedule(() -> asyncDeleteLedgerWithRetry(future, ledgerId, retry - 1),

Review Comment:
   The fix looks good. One edge case I noticed: `schedule()` can throw a `RejectedExecutionException` if the scheduler is shutting down. In that case, the original future would still be left incomplete.
   
   Would it make sense to catch the rejection here and complete the future exceptionally? I don't think this needs to block the fix, but it would close another gap in the same retry path.



-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org