merlimat commented on a change in pull request #9286:
URL: https://github.com/apache/pulsar/pull/9286#discussion_r562759077



##########
File path: 
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java
##########
@@ -207,7 +207,7 @@ public void accept(Notification n) {
 
     @Override
     public void close() throws Exception {
-        executor.shutdownNow();
+        executor.shutdown();

Review comment:
       Why do we need this change? The `shutdownNow()` will ensure that the 
thread is interrupted if it was waiting on anything. I think that, as a general 
rule, it should be preferable to wait for the thread to exit on its own.

##########
File path: 
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java
##########
@@ -542,8 +543,17 @@ public void closeFailed(ManagedLedgerException exception, 
Object ctx) {
         }
 
         scheduledExecutor.shutdownNow();
-        orderedExecutor.shutdownNow();
+        if (!scheduledExecutor.awaitTermination(30, TimeUnit.SECONDS)) {
+           log.error("Cannot stop scheduledExecutor gracefully.");
+        }
+        orderedExecutor.shutdown();

Review comment:
       Why not: 
   ```suggestion
           orderedExecutor.shutdownNow();
   ```

##########
File path: 
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java
##########
@@ -512,6 +512,7 @@ public void shutdown() throws InterruptedException, 
ManagedLedgerException {
             ledger.asyncClose(new AsyncCallbacks.CloseCallback() {
                 @Override
                 public void closeComplete(Object ctx) {
+                    log.debug("[{}] Closed managed ledger.", ledger.getName());

Review comment:
       Same as above

##########
File path: 
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java
##########
@@ -408,7 +408,7 @@ public void initializeFailed(ManagedLedgerException e) {
                         pendingLedger.ledger.asyncClose(new CloseCallback() {
                             @Override
                             public void closeComplete(Object ctx) {
-                                // no-op
+                                log.debug("[{}] Closed pending initializer 
managed ledger.", name);

Review comment:
       `if (log.isDebugEnabled()) { ...} `




----------------------------------------------------------------
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:
[email protected]


Reply via email to