poorbarcode commented on code in PR #20649:
URL: https://github.com/apache/pulsar/pull/20649#discussion_r1250543824


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
 
     @Override
     public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
-        executor.execute(() -> internalTrimConsumedLedgers(promise));
+        scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
     }
 
     public void trimConsumedLedgersInBackground(boolean isTruncate, 
CompletableFuture<?> promise) {

Review Comment:
   > This lock is segmented and does not block each producer for a long time.
   
   Yes, the task `trim ledgers` has two sub-tasks: Memory changes and Persist 
changes, and Memory changes would take the lock, Persist changes is an async 
task that will not take the lock.
   
   But the sub-task Persist changes will not use the thread `executor`, 
right?<sup>[Q-1]</sup>
   
   if yes<sup>[Q-1]</sup>, this patch doesn't have much effect on this issue, 
right?<sup>[Q-2]</sup>



##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2451,15 +2451,15 @@ private void trimConsumedLedgersInBackground() {
 
     @Override
     public void trimConsumedLedgersInBackground(CompletableFuture<?> promise) {
-        executor.execute(() -> internalTrimConsumedLedgers(promise));
+        scheduledExecutor.execute(() -> internalTrimConsumedLedgers(promise));
     }
 
     public void trimConsumedLedgersInBackground(boolean isTruncate, 
CompletableFuture<?> promise) {

Review Comment:
   > This lock is segmented and does not block each producer for a long time.
   
   Yes, the task `trim ledgers` has two sub-tasks: Memory changes and Persist 
changes, and Memory changes would take the lock, Persist changes is an async 
task that will not take the lock.
   
   ---
   
   But the sub-task Persist changes will not use the thread `executor`, 
right?<sup>[Q-1]</sup>
   
   ---
   
   if yes<sup>[Q-1]</sup>, this patch doesn't have much effect on this issue, 
right?<sup>[Q-2]</sup>



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

Reply via email to