You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/06/14 08:00:51 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request #10911: [Docs] Clarify ledger rollover criteria: address size and min time
michaeljmarshall opened a new pull request #10911:
URL: https://github.com/apache/pulsar/pull/10911
### Motivation
The pulsar broker config contains incorrect information regarding what triggers a ledger rollover. Fix the documentation.
### Modifications
In making this documentation update, I am assuming the code is correct as is. I link to the code below.
Update all relevant `.conf` files, update the website docs for all versions (the core logic hasn't changed since the initial import into GitHub), and update a single java annotation.
### Question about the implementation
Technically, the logic in the code below is such that if the `managedLedgerMinLedgerRolloverTimeMinutes` is greater than the `managedLedgerMaxLedgerRolloverTimeMinutes` time, the min time trumps the max time and the ledger will not rollover until the min time has passed. Is this logic desired? It is an invalid configuration to have `managedLedgerMinLedgerRolloverTimeMinutes > managedLedgerMaxLedgerRolloverTimeMinutes`, but it doesn't look like there is an actual check to ensure validity of this config. Is it worth giving the `managedLedgerMaxLedgerRolloverTimeMinutes` precedence?
### Verifying this change
The logic for ledger rollover is here, and upon looking at git blame, the substance of the code is from the initial import commit by @merlimat:
https://github.com/apache/pulsar/blob/6c03154ff29868181124a0a1a81e9ea09b22a9b0/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L3359-L3381
--
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
[GitHub] [pulsar] michaeljmarshall commented on pull request #10911: [Docs] Clarify ledger rollover criteria: address size and min time
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #10911:
URL: https://github.com/apache/pulsar/pull/10911#issuecomment-860010134
@merlimat @sijie @congbobo184 - PTAL and confirm this is a valid interpretation of the code. This documentation has been in place for a long time, so I want to ensure it is correct before we change it. Thanks.
--
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
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #10911: [Docs] Clarify ledger rollover criteria: address size and min time
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #10911:
URL: https://github.com/apache/pulsar/pull/10911#discussion_r651420584
##########
File path: conf/broker.conf
##########
@@ -892,10 +892,11 @@ managedLedgerCursorBackloggedThreshold=1000
managedLedgerDefaultMarkDeleteRateLimit=1.0
# Max number of entries to append to a ledger before triggering a rollover
-# A ledger rollover is triggered on these conditions
-# * Either the max rollover time has been reached
-# * or max entries have been written to the ledger and at least min-time
-# has passed
+# A ledger rollover is triggered after the min rollover time has passed
+# and one of the following conditions is true:
Review comment:
I believe `and` is correct.
As I wrote in the description, I wrote this documentation assuming the logic in the code is correct. If we want to change the code, this documentation will need to change, too.
Here is my understanding of the Java. The first check is to evaluate the following: `spaceQuotaReached || maxLedgerTimeReached`. If it is false, it the ledger is not rolled. If it is true, the next step is to evaluate `config.getMinimumRolloverTimeMs() > 0`. If it is false, the method returns true and the ledger is rolled. If it returns true, it proceeds to return the result of `timeSinceLedgerCreationMs > config.getMinimumRolloverTimeMs()`. As such, the ledger will only be rolled once the min rollover time has passed. Technically, if you misconfigure pulsar, you could set the min rollover time to greater than the max rollover time, and the ledger would not rollover until the min rollover time passed. Once the min rollover time has passed, any of the three conditions for `spaceQuotaReached || maxLedgerTimeReached` (note that `spaceQuotaReached` is composed of two conditions) will cause the ledger to rollover.
--
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
[GitHub] [pulsar] Anonymitaet commented on pull request #10911: [Docs] Clarify ledger rollover criteria: address size and min time
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #10911:
URL: https://github.com/apache/pulsar/pull/10911#issuecomment-863926398
@sijie @congbobo184 thanks for your confirmation.
@michaeljmarshall would you like to fix the same issue in the 2.8.0 doc? thanks
--
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
[GitHub] [pulsar] Anonymitaet merged pull request #10911: [Docs] Clarify ledger rollover criteria: address size and min time
Posted by GitBox <gi...@apache.org>.
Anonymitaet merged pull request #10911:
URL: https://github.com/apache/pulsar/pull/10911
--
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
[GitHub] [pulsar] michaeljmarshall commented on pull request #10911: [Docs] Clarify ledger rollover criteria: address size and min time
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #10911:
URL: https://github.com/apache/pulsar/pull/10911#issuecomment-864502766
> @michaeljmarshall would you like to fix the same issue in the 2.8.0 doc? thanks
@Anonymitaet - done, thanks for catching that.
--
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
[GitHub] [pulsar] Anonymitaet commented on a change in pull request #10911: [Docs] Clarify ledger rollover criteria: address size and min time
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #10911:
URL: https://github.com/apache/pulsar/pull/10911#discussion_r651424118
##########
File path: conf/broker.conf
##########
@@ -892,10 +892,11 @@ managedLedgerCursorBackloggedThreshold=1000
managedLedgerDefaultMarkDeleteRateLimit=1.0
# Max number of entries to append to a ledger before triggering a rollover
-# A ledger rollover is triggered on these conditions
-# * Either the max rollover time has been reached
-# * or max entries have been written to the ledger and at least min-time
-# has passed
+# A ledger rollover is triggered after the min rollover time has passed
+# and one of the following conditions is true:
Review comment:
@michaeljmarshall thanks for your detailed explanations.
@congbobo184 could you please help review?
--
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
[GitHub] [pulsar] sijie commented on a change in pull request #10911: [Docs] Clarify ledger rollover criteria: address size and min time
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #10911:
URL: https://github.com/apache/pulsar/pull/10911#discussion_r654158040
##########
File path: conf/broker.conf
##########
@@ -892,10 +892,11 @@ managedLedgerCursorBackloggedThreshold=1000
managedLedgerDefaultMarkDeleteRateLimit=1.0
# Max number of entries to append to a ledger before triggering a rollover
-# A ledger rollover is triggered on these conditions
-# * Either the max rollover time has been reached
-# * or max entries have been written to the ledger and at least min-time
-# has passed
+# A ledger rollover is triggered after the min rollover time has passed
+# and one of the following conditions is true:
Review comment:
@Anonymitaet that's correct to me.
--
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
[GitHub] [pulsar] Anonymitaet commented on a change in pull request #10911: [Docs] Clarify ledger rollover criteria: address size and min time
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #10911:
URL: https://github.com/apache/pulsar/pull/10911#discussion_r651401656
##########
File path: conf/broker.conf
##########
@@ -892,10 +892,11 @@ managedLedgerCursorBackloggedThreshold=1000
managedLedgerDefaultMarkDeleteRateLimit=1.0
# Max number of entries to append to a ledger before triggering a rollover
-# A ledger rollover is triggered on these conditions
-# * Either the max rollover time has been reached
-# * or max entries have been written to the ledger and at least min-time
-# has passed
+# A ledger rollover is triggered after the min rollover time has passed
+# and one of the following conditions is true:
Review comment:
double check: `and` or `or`?
`and` means it must meet two conditions (for example, `after the min rollover time has passed` and `The max rollover time has been reached`), then a ledger rollover is triggered.
--
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
[GitHub] [pulsar] michaeljmarshall commented on pull request #10911: [Docs] Clarify ledger rollover criteria: address size and min time
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #10911:
URL: https://github.com/apache/pulsar/pull/10911#issuecomment-864509828
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] Anonymitaet commented on a change in pull request #10911: [Docs] Clarify ledger rollover criteria: address size and min time
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #10911:
URL: https://github.com/apache/pulsar/pull/10911#discussion_r651401656
##########
File path: conf/broker.conf
##########
@@ -892,10 +892,11 @@ managedLedgerCursorBackloggedThreshold=1000
managedLedgerDefaultMarkDeleteRateLimit=1.0
# Max number of entries to append to a ledger before triggering a rollover
-# A ledger rollover is triggered on these conditions
-# * Either the max rollover time has been reached
-# * or max entries have been written to the ledger and at least min-time
-# has passed
+# A ledger rollover is triggered after the min rollover time has passed
+# and one of the following conditions is true:
Review comment:
double check: `and` or `or`?
`and` means it must meets two conditions (for example, `after the min rollover time has passed` and `The max rollover time has been reached`), then a ledger rollover is triggered.
--
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