You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "TakaHiR07 (via GitHub)" <gi...@apache.org> on 2026/09/11 07:53:17 UTC
[PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
TakaHiR07 opened a new pull request, #26542:
URL: https://github.com/apache/pulsar/pull/26542
### Motivation
If topic has multiple subscriptions, the cursor ledgers of subscriptions may rollover at the same time, which may bring additional pressure on zk.
<img width="1729" height="279" alt="企业微信截图_bf29c0d9-b029-4ff2-8bdc-30f72670d5e2" src="https://github.com/user-attachments/assets/3fbb540a-c088-4de1-a646-e44eea4fb9dd" />
Topic ledger has added the random value to avoid rollover at the same time. While cursor ledger do not have this mechanism.
https://github.com/apache/pulsar/blob/6d9f9ce2aac7b77ff9963662305260f78d03de91/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L411-L412
### Modifications
Also add the 5% random value in rollover cursor ledger.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Does this pull request potentially affect one of the following parts:
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "TakaHiR07 (via GitHub)" <gi...@apache.org>.
TakaHiR07 commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r4068148063
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -4533,7 +4533,7 @@ public ManagedLedgerConfig getConfig() {
public void setConfig(ManagedLedgerConfig config) {
this.config = config;
this.maximumRolloverTimeMs = getMaximumRolloverTimeMs(config);
- this.cursors.forEach(c -> c.setThrottleMarkDelete(config.getThrottleMarkDelete()));
+ this.cursors.forEach(c -> ((ManagedCursorImpl) c).configUpdated());
Review Comment:
Thank you for explanation, your concern is right. I think we can cache the [1, 1.05) threshold in ManagedCursorImpl to fix this problem. The code has been updated.
Besides, async initialize cursor may not update the ThrottleMarkDeleteConfig, the async problem still exist.
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "TakaHiR07 (via GitHub)" <gi...@apache.org>.
TakaHiR07 commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r4078561941
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -386,6 +391,8 @@ protected ManagedCursorImpl(BookKeeper bookkeeper, ManagedLedgerImpl ledger, Str
this.clock = getConfig().getClock();
this.lastActive = this.clock.millis();
this.lastLedgerSwitchTimestamp = this.clock.millis();
+ this.rolloverTimeJitter = 1 + ThreadLocalRandom.current().nextDouble() * 0.05;
+ this.rolloverEntriesJitter = 1 + ThreadLocalRandom.current().nextDouble() * 0.05;
Review Comment:
For compatibility, I think it is ok to keep the same semantics with previous topic-ledger rollover impletation. We can add some comments in document.
But actually someone might regard this config as representing a maximum limit that cannot be exceeded. In this point, it might be better to set [0.95, 1.0)
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "TakaHiR07 (via GitHub)" <gi...@apache.org>.
TakaHiR07 commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r4059958471
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -4533,7 +4533,7 @@ public ManagedLedgerConfig getConfig() {
public void setConfig(ManagedLedgerConfig config) {
this.config = config;
this.maximumRolloverTimeMs = getMaximumRolloverTimeMs(config);
- this.cursors.forEach(c -> c.setThrottleMarkDelete(config.getThrottleMarkDelete()));
+ this.cursors.forEach(c -> ((ManagedCursorImpl) c).configUpdated());
Review Comment:
> Could we also handle cursors that are still being initialized when setConfig() runs? asyncOpenCursor() constructs the cursor before asynchronous initialization, but only adds it to cursors after initialization completes. A config update in between misses that cursor, so the newly cached rollover thresholds remain based on the old config.
I think this is another issue, async initialize cursor may not update the config. One way to fix is do cursor.configUpdated() again after addCursor(). Should I fix the second issue in this pr?
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "TakaHiR07 (via GitHub)" <gi...@apache.org>.
TakaHiR07 commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r4002844532
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -4533,7 +4533,12 @@ public ManagedLedgerConfig getConfig() {
public void setConfig(ManagedLedgerConfig config) {
this.config = config;
this.maximumRolloverTimeMs = getMaximumRolloverTimeMs(config);
- this.cursors.forEach(c -> c.setThrottleMarkDelete(config.getThrottleMarkDelete()));
+ this.cursors.forEach(c -> {
+ if (c instanceof ManagedCursorImpl cursor) {
+ cursor.updateRolloverThresholds(config);
+ }
+ c.setThrottleMarkDelete(config.getThrottleMarkDelete());
+ });
}
Review Comment:
That is reasonable. I have been modified.
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r3988937122
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -4533,7 +4533,12 @@ public ManagedLedgerConfig getConfig() {
public void setConfig(ManagedLedgerConfig config) {
this.config = config;
this.maximumRolloverTimeMs = getMaximumRolloverTimeMs(config);
- this.cursors.forEach(c -> c.setThrottleMarkDelete(config.getThrottleMarkDelete()));
+ this.cursors.forEach(c -> {
+ if (c instanceof ManagedCursorImpl cursor) {
+ cursor.updateRolloverThresholds(config);
+ }
+ c.setThrottleMarkDelete(config.getThrottleMarkDelete());
+ });
}
Review Comment:
does this get called when the config changes?
Could we add a `updateConfig` method to ManagedCursorImpl and handle the logic there?
There's no need to add the method to ManagedCursor interface since ManagedLedgerImpl only includes ManagedCursorImpl instances.
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "Denovo1998 (via GitHub)" <gi...@apache.org>.
Denovo1998 commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r4061965776
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedCursor.java:
##########
@@ -861,19 +861,6 @@ Set<? extends Position> asyncReplayEntries(
*/
default void skipNonRecoverableLedger(long ledgerId){}
- /**
- * Returns cursor throttle mark-delete rate.
- *
- * @return
- */
- double getThrottleMarkDelete();
-
- /**
- * Update throttle mark delete rate.
- *
- */
- void setThrottleMarkDelete(double throttleMarkDelete);
-
Review Comment:
Thank you for checking. The case I was referring to is code that calls the method through an interface, for example:
```java
void configureCursor(ManagedCursor cursor) {
cursor.setThrottleMarkDelete(10.0);
}
```
Removing the declaration from ManagedCursor causes compilation to fail. If callers were compiled against the old interface, they could encounter a NoSuchMethodError at runtime, even though ManagedCursorImpl retains the method. Rebuilding the updated Pulsar tree does not cover this scenario.
Although no specific downstream caller has been identified, this still poses a compatibility risk rather than a confirmed issue. Since the interface is marked as LimitedPrivate and Stable, could we retain the existing methods and leave configUpdated() package-private?
I mentioned mocks and delegates only to illustrate that their modifications would no longer be required; preserving them was not the motivation for keeping the interface methods.
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r4002153294
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -4533,7 +4533,12 @@ public ManagedLedgerConfig getConfig() {
public void setConfig(ManagedLedgerConfig config) {
this.config = config;
this.maximumRolloverTimeMs = getMaximumRolloverTimeMs(config);
- this.cursors.forEach(c -> c.setThrottleMarkDelete(config.getThrottleMarkDelete()));
+ this.cursors.forEach(c -> {
+ if (c instanceof ManagedCursorImpl cursor) {
+ cursor.updateRolloverThresholds(config);
+ }
+ c.setThrottleMarkDelete(config.getThrottleMarkDelete());
+ });
}
Review Comment:
maybe `configUpdated` is a better method name, since the method is an event handler for config updates.
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#issuecomment-6328007577
@TakaHiR07 thanks for the change and for working through the review rounds. To help it land, I pushed two commits to the branch:
- a merge of the current `master`;
- a fix for the jitter-direction question @Denovo1998 raised. The entries jitter is now `(0.95, 1.0]`, rounded up, so `managedLedgerCursorMaxEntriesPerLedger` is never exceeded and limits below 20 entries keep their exact value. The time jitter stays `[1.0, 1.05)`, like the topic ledger rollover. There is a new test that checks the entries bound over 50 cursors.
I also updated the description. Let me know if anything there doesn't match what you intended.
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r4002142852
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -4533,7 +4533,12 @@ public ManagedLedgerConfig getConfig() {
public void setConfig(ManagedLedgerConfig config) {
this.config = config;
this.maximumRolloverTimeMs = getMaximumRolloverTimeMs(config);
- this.cursors.forEach(c -> c.setThrottleMarkDelete(config.getThrottleMarkDelete()));
+ this.cursors.forEach(c -> {
+ if (c instanceof ManagedCursorImpl cursor) {
+ cursor.updateRolloverThresholds(config);
+ }
+ c.setThrottleMarkDelete(config.getThrottleMarkDelete());
+ });
}
Review Comment:
> You mean that we add a updateConfig method to ManagedCursorImpl, which involve updateRolloverThresholds() and setThrottleMarkDelete() ?
yes
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "Denovo1998 (via GitHub)" <gi...@apache.org>.
Denovo1998 commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r4062135397
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -4533,7 +4533,7 @@ public ManagedLedgerConfig getConfig() {
public void setConfig(ManagedLedgerConfig config) {
this.config = config;
this.maximumRolloverTimeMs = getMaximumRolloverTimeMs(config);
- this.cursors.forEach(c -> c.setThrottleMarkDelete(config.getThrottleMarkDelete()));
+ this.cursors.forEach(c -> ((ManagedCursorImpl) c).configUpdated());
Review Comment:
I agree that the initialization window already exists, and any pre-existing limiter issue can be handled separately. My concern here is the new rollover-threshold caching: previously, shouldCloseLedger() read the current config directly, so missing the notification didn't leave the rollover thresholds stale. With this change, it does. That's why I think the rollover part should be addressed in this PR, without expanding the scope to fix the broader initialization issue.
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "Denovo1998 (via GitHub)" <gi...@apache.org>.
Denovo1998 commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r4046917458
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedCursor.java:
##########
@@ -861,19 +861,6 @@ Set<? extends Position> asyncReplayEntries(
*/
default void skipNonRecoverableLedger(long ledgerId){}
- /**
- * Returns cursor throttle mark-delete rate.
- *
- * @return
- */
- double getThrottleMarkDelete();
-
- /**
- * Update throttle mark delete rate.
- *
- */
- void setThrottleMarkDelete(double throttleMarkDelete);
-
Review Comment:
Could we retain these two existing methods on ManagedCursor? Keeping them only on ManagedCursorImpl would still break callers compiled against the interface, as existing binaries could fail with NoSuchMethodError and source callers would no longer compile. We can keep configUpdated() package-private while preserving the existing public API. This approach would also avoid unnecessary changes to mocks and delegates.
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -4533,7 +4533,7 @@ public ManagedLedgerConfig getConfig() {
public void setConfig(ManagedLedgerConfig config) {
this.config = config;
this.maximumRolloverTimeMs = getMaximumRolloverTimeMs(config);
- this.cursors.forEach(c -> c.setThrottleMarkDelete(config.getThrottleMarkDelete()));
+ this.cursors.forEach(c -> ((ManagedCursorImpl) c).configUpdated());
Review Comment:
Could we also handle cursors that are still being initialized when setConfig() runs? asyncOpenCursor() constructs the cursor before asynchronous initialization, but only adds it to cursors after initialization completes. A config update in between misses that cursor, so the newly cached rollover thresholds remain based on the old config. Previously, shouldCloseLedger() read the current config directly. Could we coordinate registration with config updates, or avoid caching the config-derived thresholds? A test that pauses initialization, updates the config, then completes initialization would cover this.
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "TakaHiR07 (via GitHub)" <gi...@apache.org>.
TakaHiR07 commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r4068148063
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -4533,7 +4533,7 @@ public ManagedLedgerConfig getConfig() {
public void setConfig(ManagedLedgerConfig config) {
this.config = config;
this.maximumRolloverTimeMs = getMaximumRolloverTimeMs(config);
- this.cursors.forEach(c -> c.setThrottleMarkDelete(config.getThrottleMarkDelete()));
+ this.cursors.forEach(c -> ((ManagedCursorImpl) c).configUpdated());
Review Comment:
Thank you for explanation, your concern is right. I think we can only cache the [1, 1.05) threshold in ManagedCursorImpl to fix this problem. The code has been updated.
Besides, async initialize cursor may not update the ThrottleMarkDeleteConfig, the async problem still exist.
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "TakaHiR07 (via GitHub)" <gi...@apache.org>.
TakaHiR07 commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r4002027712
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -4533,7 +4533,12 @@ public ManagedLedgerConfig getConfig() {
public void setConfig(ManagedLedgerConfig config) {
this.config = config;
this.maximumRolloverTimeMs = getMaximumRolloverTimeMs(config);
- this.cursors.forEach(c -> c.setThrottleMarkDelete(config.getThrottleMarkDelete()));
+ this.cursors.forEach(c -> {
+ if (c instanceof ManagedCursorImpl cursor) {
+ cursor.updateRolloverThresholds(config);
+ }
+ c.setThrottleMarkDelete(config.getThrottleMarkDelete());
+ });
}
Review Comment:
You mean that we add a updateConfig method to ManagedCursorImpl, which involve updateRolloverThresholds() and setThrottleMarkDelete() ?
Currently I add updateRolloverThresholds() to ManagedCursorImpl. While setThrottleMarkDelete() is still in ManagedCursor interface
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "TakaHiR07 (via GitHub)" <gi...@apache.org>.
TakaHiR07 commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r4059958471
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -4533,7 +4533,7 @@ public ManagedLedgerConfig getConfig() {
public void setConfig(ManagedLedgerConfig config) {
this.config = config;
this.maximumRolloverTimeMs = getMaximumRolloverTimeMs(config);
- this.cursors.forEach(c -> c.setThrottleMarkDelete(config.getThrottleMarkDelete()));
+ this.cursors.forEach(c -> ((ManagedCursorImpl) c).configUpdated());
Review Comment:
> Could we also handle cursors that are still being initialized when setConfig() runs? asyncOpenCursor() constructs the cursor before asynchronous initialization, but only adds it to cursors after initialization completes. A config update in between misses that cursor, so the newly cached rollover thresholds remain based on the old config.
I think this is another issue, async initialize cursor may not update the config. This issue is hard to occur. One way to fix is do cursor.configUpdated() again after addCursor(). Should I fix the second issue in this pr?
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "TakaHiR07 (via GitHub)" <gi...@apache.org>.
TakaHiR07 commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r4002844532
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -4533,7 +4533,12 @@ public ManagedLedgerConfig getConfig() {
public void setConfig(ManagedLedgerConfig config) {
this.config = config;
this.maximumRolloverTimeMs = getMaximumRolloverTimeMs(config);
- this.cursors.forEach(c -> c.setThrottleMarkDelete(config.getThrottleMarkDelete()));
+ this.cursors.forEach(c -> {
+ if (c instanceof ManagedCursorImpl cursor) {
+ cursor.updateRolloverThresholds(config);
+ }
+ c.setThrottleMarkDelete(config.getThrottleMarkDelete());
+ });
}
Review Comment:
That is reasonable. I have modified.
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "TakaHiR07 (via GitHub)" <gi...@apache.org>.
TakaHiR07 commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r4002027712
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -4533,7 +4533,12 @@ public ManagedLedgerConfig getConfig() {
public void setConfig(ManagedLedgerConfig config) {
this.config = config;
this.maximumRolloverTimeMs = getMaximumRolloverTimeMs(config);
- this.cursors.forEach(c -> c.setThrottleMarkDelete(config.getThrottleMarkDelete()));
+ this.cursors.forEach(c -> {
+ if (c instanceof ManagedCursorImpl cursor) {
+ cursor.updateRolloverThresholds(config);
+ }
+ c.setThrottleMarkDelete(config.getThrottleMarkDelete());
+ });
}
Review Comment:
You mean that we add a updateConfig method to ManagedCursorImpl, which involve updateRolloverThresholds() and setThrottleMarkDelete() ?
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "TakaHiR07 (via GitHub)" <gi...@apache.org>.
TakaHiR07 commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r4068148063
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -4533,7 +4533,7 @@ public ManagedLedgerConfig getConfig() {
public void setConfig(ManagedLedgerConfig config) {
this.config = config;
this.maximumRolloverTimeMs = getMaximumRolloverTimeMs(config);
- this.cursors.forEach(c -> c.setThrottleMarkDelete(config.getThrottleMarkDelete()));
+ this.cursors.forEach(c -> ((ManagedCursorImpl) c).configUpdated());
Review Comment:
Thank you for explanation, your concern is right. I think we can cache the [1, 1.05) threshold in ManagedCursorImpl to fix this problem. The code has been updated.
Besides, async initialize cursor may not update the ThrottleMarkDeleteConfig, async problem still exist in the other config.
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "Denovo1998 (via GitHub)" <gi...@apache.org>.
Denovo1998 commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r4072121292
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -386,6 +391,8 @@ protected ManagedCursorImpl(BookKeeper bookkeeper, ManagedLedgerImpl ledger, Str
this.clock = getConfig().getClock();
this.lastActive = this.clock.millis();
this.lastLedgerSwitchTimestamp = this.clock.millis();
+ this.rolloverTimeJitter = 1 + ThreadLocalRandom.current().nextDouble() * 0.05;
+ this.rolloverEntriesJitter = 1 + ThreadLocalRandom.current().nextDouble() * 0.05;
Review Comment:
One small question about the jitter direction:
`managedLedgerCursorMaxEntriesPerLedger` is documented as the maximum number of entries to append to a cursor ledger, but the range `[1.0, 1.05)` allows the actual threshold to exceed the configured maximum by up to 5%.
I see the topic-ledger rollover already uses the same +5% approach, so this is consistent with the existing implementation. I'm wondering whether we want the same semantics for the cursor entry limit, or if using `[0.95, 1.0]` would spread the rollover while still respecting the configured upper bound.
This isn’t necessarily a blocker, but I think it would be helpful to clarify the intended semantics here.
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "TakaHiR07 (via GitHub)" <gi...@apache.org>.
TakaHiR07 commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r4060045718
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedCursor.java:
##########
@@ -861,19 +861,6 @@ Set<? extends Position> asyncReplayEntries(
*/
default void skipNonRecoverableLedger(long ledgerId){}
- /**
- * Returns cursor throttle mark-delete rate.
- *
- * @return
- */
- double getThrottleMarkDelete();
-
- /**
- * Update throttle mark delete rate.
- *
- */
- void setThrottleMarkDelete(double throttleMarkDelete);
-
Review Comment:
which situation would break? I can compile the pulsar package after remove setThrottleMarkDelete from ManagedCursor interface.
Besides, AI suggest that keep them in mocks and delegates, seems is not a good way.
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #26542:
URL: https://github.com/apache/pulsar/pull/26542#discussion_r4249624702
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -4533,7 +4533,12 @@ public ManagedLedgerConfig getConfig() {
public void setConfig(ManagedLedgerConfig config) {
this.config = config;
this.maximumRolloverTimeMs = getMaximumRolloverTimeMs(config);
- this.cursors.forEach(c -> c.setThrottleMarkDelete(config.getThrottleMarkDelete()));
+ this.cursors.forEach(c -> {
+ if (c instanceof ManagedCursorImpl cursor) {
+ cursor.updateRolloverThresholds(config);
+ }
+ c.setThrottleMarkDelete(config.getThrottleMarkDelete());
+ });
}
Review Comment:
Thanks, this is moot now. The latest version only caches the jitter and reads the live config in `shouldCloseLedger`, so no `configUpdated` handler is needed. Resolving.
--
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
Re: [PR] [improve][broker] Add a random value upto 5% to avoid rollover multiple cursor ledgers [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat merged PR #26542:
URL: https://github.com/apache/pulsar/pull/26542
--
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