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 2023/01/18 18:33:32 UTC

[GitHub] [pulsar] poorbarcode opened a new pull request, #19275: [fix] [ml] Topics stats shows msgBacklog but there reality no backlog

poorbarcode opened a new pull request, #19275:
URL: https://github.com/apache/pulsar/pull/19275

   ### Motivation
   
   When `trim ledgers` and `create new cursor` are executed concurrently, it will cause the `readPosition` of the cursor to point to a deleted ledger.
   
   | time | `trim ledgers` | `create new cursor` |
   | --- | --- | --- |
   | 1 | | set read position and mark deleted position |
   | 2 | delete ledger | |
   | 3 | | add the cursor to `ManagedLedger.cursors` |
   
   <strong>(Highlight)</strong>Since the read position of the cursor is pointing at a deleted ledger, so deleted messages will never be consumed or acknowledged. Since the backlog in the API `topics stats` response is calculated as this: `managedLedger.entriesAddedCounter - cursor.messagesConsumedCounter`, the result is: Topics stats show `msgBacklog` but there is reality no backlog.
   - `managedLedger.entriesAddedCounter`: Pulsar will set it to `0` when creating a new managed ledger, it will increment when adding entries.
   - `cursor.messagesConsumedCounter`: Pulsar will set it to `0` when creating a new cursor, it will increment when acknowledging.
   
   For example: 
   - write entries to the managed ledger: `{1:0~1:10}...{5:0~5:10}`
     - `managedLedger.entriesAddedCounter` is `50` now
   - create a new cursor, and set the read position to `1:0`
     -  `managedLedger.entriesAddedCounter` is `0` now
   - delete ledgers `1~4`
   - consume all messages
     - can only consume the messages {5:0~5:10}, so `managedLedger.entriesAddedCounter` is `5` now
   - the `backlog` in response of `topics stats` is `50 - 5 = 45`, but there reality no backlog
   
   Sorry, I spent 4 hours trying to write a non-invasive test, but failed. <strong>(Highlight)</strong>You can reproduce by `testBacklogIfCursorCreateConcurrentWithTrimLedger` in the PR #19274
   
   https://github.com/apache/pulsar/blob/a2cdc759fc2710e4dd913eb0485d23ebcaa076a4/pulsar-broker/src/test/java/org/apache/bookkeeper/mledger/impl/StatsBackLogTest.java#L163
   
   ### Modifications
   
   If there are uninitialized cursors, execution `trim ledgers` is deferred.
   
   ### Documentation
   
   <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
   
   - [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. -->
   - [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
   - [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
   - [ ] `doc-complete` <!-- Docs have been already added -->
   
   ### Matching PR in forked repository
   
   PR in forked repository: 
   - https://github.com/poorbarcode/pulsar/pull/53


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

[GitHub] [pulsar] mattisonchao commented on a diff in pull request #19275: [fix] [ml] Topics stats shows msgBacklog but there reality no backlog

Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #19275:
URL: https://github.com/apache/pulsar/pull/19275#discussion_r1081169593


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -993,12 +993,11 @@ public synchronized void asyncOpenCursor(final String cursorName, final InitialP
             public void operationComplete() {
                 log.info("[{}] Opened new cursor: {}", name, cursor);
                 cursor.setActive();

Review Comment:
   Should we move this line to `synchronized`? I don't have a strong reason. But it looks more reasonable to put `cursor.active()` after updating the init position.(maybe we can do that when we got strong reason)
    



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

[GitHub] [pulsar] poorbarcode commented on pull request #19275: [fix] [ml] Topics stats shows msgBacklog but there reality no backlog

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #19275:
URL: https://github.com/apache/pulsar/pull/19275#issuecomment-1396748584

   @codelipenghui 
   
   > Now the solution is to avoid the race condition of cursor.initializeCursorPosition and internalTrimLedgers, right?
   
   Yes, Already fixed


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

[GitHub] [pulsar] mattisonchao merged pull request #19275: [fix] [ml] Topics stats shows msgBacklog but there reality no backlog

Posted by GitBox <gi...@apache.org>.
mattisonchao merged PR #19275:
URL: https://github.com/apache/pulsar/pull/19275


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

[GitHub] [pulsar] poorbarcode commented on pull request #19275: [fix] [ml] Topics stats shows msgBacklog but there reality no backlog

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #19275:
URL: https://github.com/apache/pulsar/pull/19275#issuecomment-1396752247

   Hi @HQebupt 
   
   >I believe the expected behavior is following : can only consume the messages {5:0~5:10}, so managedLedger.entriesAddedCounter is 5 10 now. the backlog in response of topics stats is 50 - 5 = 45 50 - 10 = 40, but there reality no backlog
   
   I wrote the wrong variable name. It has been corrected. 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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] poorbarcode commented on a diff in pull request #19275: [fix] [ml] Topics stats shows msgBacklog but there reality no backlog

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #19275:
URL: https://github.com/apache/pulsar/pull/19275#discussion_r1081086298


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -690,11 +690,11 @@ private void recoveredCursor(PositionImpl position, Map<String, Long> properties
         }
         log.info("[{}] Cursor {} recovered to position {}", ledger.getName(), name, position);
         this.cursorProperties = cursorProperties;
-        messagesConsumedCounter = -getNumberOfEntries(Range.openClosed(position, ledger.getLastPosition()));
         markDeletePosition = position;
         persistentMarkDeletePosition = position;
         inProgressMarkDeletePersistPosition = null;
         readPosition = ledger.getNextValidPosition(position);
+        messagesConsumedCounter = -getNumberOfEntries(Range.openClosed(position, ledger.getLastPosition()));

Review Comment:
   This line doesn't really have anything to do with this issue, I removed this change. Please take a look



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

[GitHub] [pulsar] HQebupt commented on pull request #19275: [fix] [ml] Topics stats shows msgBacklog but there reality no backlog

Posted by GitBox <gi...@apache.org>.
HQebupt commented on PR #19275:
URL: https://github.com/apache/pulsar/pull/19275#issuecomment-1396727013

   > consume all messages
   > - can only consume the messages {5:0~5:10}, so managedLedger.entriesAddedCounter is 5 now
   > the backlog in response of topics stats is 50 - 5 = 45, but there reality no backlog
   
   I believe the expected behavior is following. 
   ```
   can only consume the messages {5:0~5:10}, so managedLedger.entriesAddedCounter is ~~5~~ **10** now
   the backlog in response of topics stats is ~~50 - 5 = 45~~ **50 - 10 = 40**, but there reality no backlog
   ```


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

[GitHub] [pulsar] poorbarcode commented on pull request #19275: [fix] [ml] Topics stats shows msgBacklog but there reality no backlog

Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #19275:
URL: https://github.com/apache/pulsar/pull/19275#issuecomment-1397474820

   /pulsarbot rerun-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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] HQebupt commented on a diff in pull request #19275: [fix] [ml] Topics stats shows msgBacklog but there reality no backlog

Posted by GitBox <gi...@apache.org>.
HQebupt commented on code in PR #19275:
URL: https://github.com/apache/pulsar/pull/19275#discussion_r1081068509


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -690,11 +690,11 @@ private void recoveredCursor(PositionImpl position, Map<String, Long> properties
         }
         log.info("[{}] Cursor {} recovered to position {}", ledger.getName(), name, position);
         this.cursorProperties = cursorProperties;
-        messagesConsumedCounter = -getNumberOfEntries(Range.openClosed(position, ledger.getLastPosition()));
         markDeletePosition = position;
         persistentMarkDeletePosition = position;
         inProgressMarkDeletePersistPosition = null;
         readPosition = ledger.getNextValidPosition(position);
+        messagesConsumedCounter = -getNumberOfEntries(Range.openClosed(position, ledger.getLastPosition()));

Review Comment:
   Why change the order of this line? it doesn't seem to help.



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

[GitHub] [pulsar] mattisonchao commented on a diff in pull request #19275: [fix] [ml] Topics stats shows msgBacklog but there reality no backlog

Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #19275:
URL: https://github.com/apache/pulsar/pull/19275#discussion_r1081169593


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -993,12 +993,11 @@ public synchronized void asyncOpenCursor(final String cursorName, final InitialP
             public void operationComplete() {
                 log.info("[{}] Opened new cursor: {}", name, cursor);
                 cursor.setActive();

Review Comment:
   Should we move this line to `synchromized`?



##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -993,12 +993,11 @@ public synchronized void asyncOpenCursor(final String cursorName, final InitialP
             public void operationComplete() {
                 log.info("[{}] Opened new cursor: {}", name, cursor);
                 cursor.setActive();

Review Comment:
   Should we move this line to `synchronized`?



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

[GitHub] [pulsar] codecov-commenter commented on pull request #19275: [fix] [ml] Topics stats shows msgBacklog but there reality no backlog

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #19275:
URL: https://github.com/apache/pulsar/pull/19275#issuecomment-1396877999

   # [Codecov](https://codecov.io/gh/apache/pulsar/pull/19275?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#19275](https://codecov.io/gh/apache/pulsar/pull/19275?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bed955e) into [master](https://codecov.io/gh/apache/pulsar/commit/334c3a541f00c08aa2f378a6cc45f37cbeb54a5c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (334c3a5) will **decrease** coverage by `2.42%`.
   > The diff coverage is `8.69%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pulsar/pull/19275/graphs/tree.svg?width=650&height=150&src=pr&token=acYqCpsK9J&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/pulsar/pull/19275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #19275      +/-   ##
   ============================================
   - Coverage     47.04%   44.62%   -2.42%     
   - Complexity     9190    18464    +9274     
   ============================================
     Files           607     1824    +1217     
     Lines         57677   136749   +79072     
     Branches       6007    15047    +9040     
   ============================================
   + Hits          27132    61030   +33898     
   - Misses        27598    68747   +41149     
   - Partials       2947     6972    +4025     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | inttests | `19.40% <6.52%> (?)` | |
   | systests | `24.51% <1.63%> (?)` | |
   | unittests | `35.88% <8.15%> (-11.16%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/pulsar/pull/19275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...rg/apache/pulsar/broker/delayed/bucket/Bucket.java](https://codecov.io/gh/apache/pulsar/pull/19275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9kZWxheWVkL2J1Y2tldC9CdWNrZXQuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...r/delayed/bucket/BucketDelayedDeliveryTracker.java](https://codecov.io/gh/apache/pulsar/pull/19275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9kZWxheWVkL2J1Y2tldC9CdWNrZXREZWxheWVkRGVsaXZlcnlUcmFja2VyLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...layed/bucket/CombinedSegmentDelayedIndexQueue.java](https://codecov.io/gh/apache/pulsar/pull/19275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9kZWxheWVkL2J1Y2tldC9Db21iaW5lZFNlZ21lbnREZWxheWVkSW5kZXhRdWV1ZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...ulsar/broker/delayed/bucket/DelayedIndexQueue.java](https://codecov.io/gh/apache/pulsar/pull/19275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9kZWxheWVkL2J1Y2tldC9EZWxheWVkSW5kZXhRdWV1ZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [.../pulsar/broker/delayed/bucket/ImmutableBucket.java](https://codecov.io/gh/apache/pulsar/pull/19275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9kZWxheWVkL2J1Y2tldC9JbW11dGFibGVCdWNrZXQuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...he/pulsar/broker/delayed/bucket/MutableBucket.java](https://codecov.io/gh/apache/pulsar/pull/19275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9kZWxheWVkL2J1Y2tldC9NdXRhYmxlQnVja2V0LmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...ed/bucket/TripleLongPriorityDelayedIndexQueue.java](https://codecov.io/gh/apache/pulsar/pull/19275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9kZWxheWVkL2J1Y2tldC9UcmlwbGVMb25nUHJpb3JpdHlEZWxheWVkSW5kZXhRdWV1ZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...rg/apache/pulsar/broker/service/BrokerService.java](https://codecov.io/gh/apache/pulsar/pull/19275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Jyb2tlclNlcnZpY2UuamF2YQ==) | `43.72% <0.00%> (-14.16%)` | :arrow_down: |
   | [...che/pulsar/websocket/AbstractWebSocketHandler.java](https://codecov.io/gh/apache/pulsar/pull/19275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLXdlYnNvY2tldC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL3dlYnNvY2tldC9BYnN0cmFjdFdlYlNvY2tldEhhbmRsZXIuamF2YQ==) | `40.71% <56.25%> (ø)` | |
   | [...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java](https://codecov.io/gh/apache/pulsar/pull/19275?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9pbXBsL01hbmFnZWRMZWRnZXJJbXBsLmphdmE=) | `73.26% <100.00%> (ø)` | |
   | ... and [1476 more](https://codecov.io/gh/apache/pulsar/pull/19275?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   


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