[GitHub] [pulsar] sijie commented on a change in pull request #7506: [Broker] Timeout opening managed ledger operation …

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

sijie commented on a change in pull request #7506:
URL: https://github.com/apache/pulsar/pull/7506#discussion_r453106816



##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java
##########
@@ -320,18 +335,32 @@ public void asyncOpen(final String name, final ManagedLedgerConfig config, final
 
         // If the ledger state is bad, remove it from the map.
         CompletableFuture<ManagedLedgerImpl> existingFuture = ledgers.get(name);
-        if (existingFuture != null && existingFuture.isDone()) {
-            try {
-                ManagedLedgerImpl l = existingFuture.get();
-                if (l.getState().equals(State.Fenced.toString()) || l.getState().equals(State.Closed.toString())) {
-                    // Managed ledger is in unusable state. Recreate it.
-                    log.warn("[{}] Attempted to open ledger in {} state. Removing from the map to recreate it", name,
+        if (existingFuture != null) {
+            if (existingFuture.isDone()) {
+                try {
+                    ManagedLedgerImpl l = existingFuture.get();
+                    if (l.getState().equals(State.Fenced.toString()) || l.getState().equals(State.Closed.toString())) {
+                        // Managed ledger is in unusable state. Recreate it.
+                        log.warn("[{}] Attempted to open ledger in {} state. Removing from the map to recreate it", name,
                             l.getState());
-                    ledgers.remove(name, existingFuture);
+                        ledgers.remove(name, existingFuture);
+                    }
+                } catch (Exception e) {
+                    // Unable to get the future
+                    log.warn("[{}] Got exception while trying to retrieve ledger", name, e);
                 }
-            } catch (Exception e) {
-                // Unable to get the future
-                log.warn("[{}] Got exception while trying to retrieve ledger", name, e);
+            } else {
+                PendingInitializeManagedLedger pendingLedger = pendingInitializeLedgers.get(name);

Review comment:
       We can do that. However, I didn't go down that route because of the following reason:
   
   we need to keep the ManageLedger reference and to close it to release resources. Because the initialization involves a long pipeline including opening managed ledger and cursors. The behavior we observed is that the operation is stuck at opening cursors. So if we don't attempt to close the ManagedLedger instance, it can result in resource leaking. 
   
   With that being said, we need to keep a reference to ManagedLedgerImpl along with the CompletableFuture. Hence I chose the current implementation.
   
   Another reason is to allow checking the timestamp proactively. I fixed a couple of issues before that NPE is thrown between creating a Future and registering error handling logic. I would like to have a mechanism in place that can fix any potential bugs by proactively checking if a CompletableFuture timed out.




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

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