[GitHub] [pulsar] eolivelli commented on a diff in pull request #17228: [fix][ml] refresh the ledgers map when the offload complete failed

[GitBox <gi...@apache.org>]

eolivelli commented on code in PR #17228:
URL: https://github.com/apache/pulsar/pull/17228#discussion_r962986673


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -3189,15 +3247,32 @@ private CompletableFuture<Void> completeLedgerInfoForOffloaded(long ledgerId, UU
                                        }
                                    })
             .whenComplete((result, exception) -> {
-                    if (exception == null) {
+                if (injection != null) {
+                    lastOffloadCompleteFailed = true;
+                    refreshedIfOffloadCompleteFailed = false;
+                    injection.throwException(ledgerId);
+                }
+                if (exception == null) {
                         log.info("[{}] End Offload. ledger={}, uuid={}", name, ledgerId, uuid);
                     } else {
+                        lastOffloadCompleteFailed = true;
+                        refreshedIfOffloadCompleteFailed = false;
                         log.warn("[{}] Failed to complete offload of ledger {}, uuid {}",
                                  name, ledgerId, uuid, exception);
                     }
                 });
     }
 
+    private Injection injection;

Review Comment:
   please do not add this mechanism on core classes.
   we can use Mockito to interact with the internals and simulate problems



##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -220,6 +220,10 @@ public class ManagedLedgerImpl implements ManagedLedger, CreateCallback {
     private long lastOffloadLedgerId = 0;
     private long lastOffloadSuccessTimestamp = 0;
     private long lastOffloadFailureTimestamp = 0;
+    @Getter
+    private boolean lastOffloadCompleteFailed = false;

Review Comment:
   I don't think that we are handing correctly concurrent access to these fields
   they should be at least `volatile`



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