Skip to content

Conversation

summeriiii
Copy link
Contributor

Fixes #23239

Motivation

Briefly describe the issue:

Consumer consumed all messages and then the msgBacklog is 0, but after topic unload, the msgBacklog turned to be 1 instead of 0 which is not what we expected. The detail is in test NonDurableSubscriptionTest#testNonDurableSubscriptionBackLogAfterTopicUnload.

Root cause:

I analysed the related code, and found this issue was introduced by #4331. In the #4331,
if (((BatchMessageIdImpl) msgId).getBatchIndex() >= 0) { was deleted directly, which cause none batch message will also execute the step entryId = msgId.getEntryId() - 1;. Because of this, the entryId reduced by 1, then the msgBacklog turned to be 1 instead of 0.

In the #4331, we added ReaderBuilder#startMessageIdInclusive interface to allow create Reader which read containes startMessageId, but it has not been implemented correctly. We should add the field resetIncludeHead in CommandSubscribe to implement it.

Modifications

  • CommandSubscribe add the field resetIncludeHead, and when use the ReaderBuilder#startMessageIdInclusive, this param is true while other is false.
  • Persist#getNonDurableSubscription method add the judge condition (msgId.getBatchIndex() >= 0 || resetIncludeHead), entryId -1 will execute Only when msg is batch or the resetIncludeHead is true.
               if (ledgerId >= 0 && entryId >= 0
                        && msgId instanceof BatchMessageIdImpl
                        && (msgId.getBatchIndex() >= 0 || resetIncludeHead)) {
                    // When the start message is relative to a batch, we need to take one step back on the previous
                    // message,
                    // because the "batch" might not have been consumed in its entirety.
                    // The client will then be able to discard the first messages if needed.
                    entryId = msgId.getEntryId() - 1;
                }

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 13, 2024
@summeriiii
Copy link
Contributor Author

@lovelle @lhotari please take a look :)

@Technoboy- Technoboy- added this to the 4.0.0 milestone Sep 19, 2024
@Technoboy- Technoboy- added ready-to-test and removed doc-not-needed Your PR changes do not impact docs labels Sep 19, 2024
@Technoboy- Technoboy- closed this Sep 19, 2024
@Technoboy- Technoboy- reopened this Sep 19, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 19, 2024
@summeriiii summeriiii force-pushed the fix_backlog_incorrect branch 2 times, most recently from 691386d to 80c923e Compare September 23, 2024 07:19
@Technoboy-
Copy link
Contributor

re-run the failed test

if (ledgerId >= 0 && entryId >= 0
&& msgId instanceof BatchMessageIdImpl) {
&& msgId instanceof BatchMessageIdImpl
&& (msgId.getBatchIndex() >= 0 || resetIncludeHead)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding resetIncludeHead should start a PIP. right now, we can only fix by adding msgId.getBatchIndex() >= 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if we just add the msgId.getBatchIndex() >= 0, test of method ReaderBuilder#startMessageIdInclusive will fail

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will draft a pip to fix this issue later

@summeriiii summeriiii changed the title [fix][broker] Fix NonDurable Subscription msgBackLog incorrect after … [fix][broker] Fix NonDurable Subscription msgBackLog incorrect after topic unload Oct 9, 2024
@lhotari lhotari modified the milestones: 4.0.0, 4.1.0 Oct 11, 2024
@summeriiii summeriiii force-pushed the fix_backlog_incorrect branch from 80c923e to 54b4feb Compare October 15, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs ready-to-test release/4.0.8

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Non durable subscription backlog is wrong after topic unload

4 participants