Trigger rollover when meeting maxLedgerRolloverTimeMinutes #7111

wuzhanpeng · 2020-05-30T04:22:42Z

This pull request resolves #7184

Motivation

This pull request implements a monitor thread to check if the current topic ledger meets the constraint of managedLedgerMaxLedgerRolloverTimeMinutes and trigger a rollover to make the configuration take effect. Another important idea here is, if we trigger a rollover, we can close the current ledger so that we can release the storage of current ledger btw. Because for some less commonly used topics, the current ledger data is likely to have expired, and the current rollover logic will only be triggered when adding a new entry. Obviously, this will result in a waste of disk space.

Expected behaviors

The monitor thread will be scheduled at fix time interval and the interval is set to managedLedgerMaxLedgerRolloverTimeMinutes. Each inspection will make two judgments at the same time, i.e. currentLedgerEntries > 0 and currentLedgerIsFull(). When the number of current entry is equal to 0, it will not trigger a new rollover and we use this to reduce the ledger's creation.

Modifications

The main modification took place in ManagedLedgerImpl
In addition, a check thread was added in the BrokerService

maybe related to #6935

…utes

merlimat · 2020-05-30T06:49:37Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedger.java

+    /**
+     * Roll current ledger if it is full
+     */
+    void rollCurrentLedgerIfFull();


Why do we need to expose this in the interface, it should be better to keep in implementation details

I hope to be able to consult you here for advice, because I am not sure whether doing type casting in the BrokerService is a suitable implementation. Maybe I can modify with:

((ManagedLedgerImpl) managedLedger).rollCurrentLedgerIfFull();

I was thinking that this is a managedLedger internal task, for which the BrokerService shouldn't be concerned. For that it would be better to handle in the ManagedLedgerFactoryImpl, to go through all open managed ledger instances and check if a rollover has to be forced.

Now I feel a little confused. If we treat rollover as an internal task, should consumedLedgersMonitor or backlogQuotaChecker be internal tasks as well? Or is my understanding biased?

@wuzhanpeng I think you can add an issue to track the monitor that outside the managed ledger but should be maintained by managed ledger own.

merlimat · 2020-05-30T06:50:38Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

+                    }
+
+                    ledgerClosed(lh);
+                    createLedgerAfterClosed();


We don't need initiate the creation of a ledger at this point. We can stay in LedgerClosed state until a new write comes in.

Yes in current code logic we can stay in closed state until the next entry comes in, however if we choose for waiting, the last created topic ledger will be never removed beacause the trimming stratege will not remove the current ledger. In such scenario, we will maintain a lot of useless data if the topic is no longer being used. Moreover, it may cause disk problem if we keep a lot mount of discarded topics in the cluster.

Ok, I think we can change the current logic. If the current ledger is closed, we can delete it. I'm not sure is there any problems with this change. @merlimat Could you please help check this?

merlimat · 2020-05-30T06:51:46Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

@@ -277,6 +278,8 @@ public BrokerService(PulsarService pulsar) throws Exception {
            Executors.newSingleThreadScheduledExecutor(new DefaultThreadFactory("pulsar-publish-buffer-monitor"));
        this.consumedLedgersMonitor = Executors
                .newSingleThreadScheduledExecutor(new DefaultThreadFactory("consumed-Ledgers-monitor"));
+        this.ledgerFullMonitor =


This thread is not being stopped. Potentially we could also reuse an existing executor.

+1 @wuzhanpeng can we re-use an existing executor? Otherwise, we end up creating a lot of executors.

merlimat · 2020-05-30T06:52:28Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

@@ -485,6 +489,12 @@ protected void startConsumedLedgersMonitor() {
        }
    }

+    protected void startLedgerFullMonitor() {
+        int interval = pulsar().getConfiguration().getManagedLedgerMaxLedgerRolloverTimeMinutes();
+        ledgerFullMonitor.scheduleAtFixedRate(safeRun(this::checkLedgerFull),


the recurring task needs to be cancelled when BrokerService is closed

I newly add a shutdown for the monitor when BrokerService is closed

codelipenghui · 2020-06-04T04:19:45Z

move to 2.7.0 first.

jiazhai · 2020-06-04T13:17:23Z

@merlimat Would you please help review it again?

…en_triggering_maxLedgerRolloverTimeMinutes

codelipenghui

The change looks good to me, I just left a minor comment.

codelipenghui · 2020-06-05T02:56:57Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

+                    }
+
+                    ledgerClosed(lh);
+                    createLedgerAfterClosed();


Ok, I think we can change the current logic. If the current ledger is closed, we can delete it. I'm not sure is there any problems with this change. @merlimat Could you please help check this?

sijie · 2020-06-08T01:10:37Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

@@ -277,6 +278,8 @@ public BrokerService(PulsarService pulsar) throws Exception {
            Executors.newSingleThreadScheduledExecutor(new DefaultThreadFactory("pulsar-publish-buffer-monitor"));
        this.consumedLedgersMonitor = Executors
                .newSingleThreadScheduledExecutor(new DefaultThreadFactory("consumed-Ledgers-monitor"));
+        this.ledgerFullMonitor =


+1 @wuzhanpeng can we re-use an existing executor? Otherwise, we end up creating a lot of executors.

This pull request resolves apache#7184 ### Motivation This pull request implements a monitor thread to check if the current topic ledger meets the constraint of `managedLedgerMaxLedgerRolloverTimeMinutes` and trigger a rollover to make the configuration take effect. Another important idea here is, if we trigger a rollover, we can close the current ledger so that we can release the storage of current ledger btw. Because for some less commonly used topics, the current ledger data is likely to have expired, and the current rollover logic will only be triggered when adding a new entry. Obviously, this will result in a waste of disk space. ### Expected behaviors The monitor thread will be scheduled at fix time interval and the interval is set to `managedLedgerMaxLedgerRolloverTimeMinutes`. Each inspection will make two judgments at the same time, i.e. `currentLedgerEntries > 0` and `currentLedgerIsFull()`. When the number of current entry is equal to 0, it will not trigger a new rollover and we use this to reduce the ledger's creation. ### Modifications - The main modification took place in `ManagedLedgerImpl` - In addition, a check thread was added in the `BrokerService` maybe related to apache#6935

…0087) This pull request resolves #10086 ## Motivation Although #7111 and #9136 solved the problems of 1. current ledger is full and cannot be rolled 2. cursor has subscribed to an expired ledger, which makes the cleaning thread unable to clean up respectively. However, when we close and open a managed-ledger(such like brokers shutdown unexpectedly or topics unload due to rebalance), the managed-ledger will create an empty ledger after initialization causing the current cleanup logic failed to take effect. Therefore I think we need a more unified entrance to solve the cursor update problem, and then further solve the problem of clearing expired ledgers. ## Expected Behavior 1. Able to cleanup expired data after managed-ledger re-open 2. A more unified entrance to implement cursor update ## Modification 1. move the cursor update logic to `internalTrimConsumedLedgers` from `ManagedLedgerImpl`'s callback `createComplete` 2. update `lastConfirmedEntry` if necessary when updating cursors

…ache#10087) This pull request resolves apache#10086 ## Motivation Although apache#7111 and apache#9136 solved the problems of 1. current ledger is full and cannot be rolled 2. cursor has subscribed to an expired ledger, which makes the cleaning thread unable to clean up respectively. However, when we close and open a managed-ledger(such like brokers shutdown unexpectedly or topics unload due to rebalance), the managed-ledger will create an empty ledger after initialization causing the current cleanup logic failed to take effect. Therefore I think we need a more unified entrance to solve the cursor update problem, and then further solve the problem of clearing expired ledgers. ## Expected Behavior 1. Able to cleanup expired data after managed-ledger re-open 2. A more unified entrance to implement cursor update ## Modification 1. move the cursor update logic to `internalTrimConsumedLedgers` from `ManagedLedgerImpl`'s callback `createComplete` 2. update `lastConfirmedEntry` if necessary when updating cursors

wuzhanpeng added 2 commits May 29, 2020 20:08

add feature: enable rollover when triggering maxLedgerRolloverTimeMin…

ee2fc25

…utes

add supplemental test cases

b155894

merlimat reviewed May 30, 2020

View reviewed changes

codelipenghui requested review from sijie, codelipenghui, merlimat and jiazhai May 31, 2020 02:01

codelipenghui assigned wuzhanpeng May 31, 2020

codelipenghui added the area/broker label May 31, 2020

codelipenghui added this to the 2.6.0 milestone May 31, 2020

shutdown monitor when BrokerService is closed

33f0615

codelipenghui modified the milestones: 2.6.0, 2.7.0 Jun 4, 2020

codelipenghui modified the milestones: 2.7.0, 2.6.0 Jun 5, 2020

Merge remote-tracking branch 'apache/master' into feature_rollover_wh…

9d225e4

…en_triggering_maxLedgerRolloverTimeMinutes

codelipenghui approved these changes Jun 5, 2020

View reviewed changes

sijie approved these changes Jun 8, 2020

View reviewed changes

codelipenghui merged commit fa2da42 into apache:master Jun 8, 2020

codelipenghui mentioned this pull request Jun 8, 2020

Make ledger rollover check task internal #7195

Closed

This was referenced Jun 8, 2020

ISSUE-7195: Make ledger rollover check task internal streamnative/pulsar-archived#1062

Closed

[discussion] Pulsar release 2.6.0 #5819

Closed

wangjialing218 mentioned this pull request Dec 25, 2020

Closed ledger did not deleted after retention period when no incoming traffic #9057

Closed

sijie mentioned this pull request Dec 25, 2020

ISSUE-9057: Closed ledger did not deleted after retention period when no incoming traffic streamnative/pulsar-archived#1944

Closed

MarvinCai mentioned this pull request Mar 19, 2021

Close current ledger when retention reached #9060

Closed

wuzhanpeng mentioned this pull request Mar 30, 2021

[BUGFIX] cannot cleanup expired data after managed-ledger restart #10086

Closed

wuzhanpeng mentioned this pull request Mar 30, 2021

[BUGFIX] cannot cleanup expired data after managed-ledger restart #10087

Merged

sijie mentioned this pull request Mar 30, 2021

ISSUE-10086: [BUGFIX] cannot cleanup expired data after managed-ledger restart streamnative/pulsar-archived#2349

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trigger rollover when meeting maxLedgerRolloverTimeMinutes #7111

Trigger rollover when meeting maxLedgerRolloverTimeMinutes #7111

wuzhanpeng commented May 30, 2020 •

edited

Loading

merlimat May 30, 2020

wuzhanpeng Jun 1, 2020

merlimat Jun 1, 2020

wuzhanpeng Jun 2, 2020

codelipenghui Jun 5, 2020

merlimat May 30, 2020

wuzhanpeng Jun 1, 2020

codelipenghui Jun 5, 2020

merlimat May 30, 2020

sijie Jun 8, 2020

merlimat May 30, 2020

wuzhanpeng Jun 1, 2020

codelipenghui commented Jun 4, 2020

jiazhai commented Jun 4, 2020

codelipenghui left a comment

codelipenghui Jun 5, 2020

sijie Jun 8, 2020

Trigger rollover when meeting maxLedgerRolloverTimeMinutes #7111

Trigger rollover when meeting maxLedgerRolloverTimeMinutes #7111

Conversation

wuzhanpeng commented May 30, 2020 • edited Loading

Motivation

Expected behaviors

Modifications

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codelipenghui commented Jun 4, 2020

jiazhai commented Jun 4, 2020

codelipenghui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuzhanpeng commented May 30, 2020 •

edited

Loading