Skip to content

[fix][broker] Fix skip message API when hole messages exists#20326

Merged
Technoboy- merged 2 commits into
apache:masterfrom
crossoverJie:fix-issue-20262
Jun 1, 2023
Merged

[fix][broker] Fix skip message API when hole messages exists#20326
Technoboy- merged 2 commits into
apache:masterfrom
crossoverJie:fix-issue-20262

Conversation

@crossoverJie

@crossoverJie crossoverJie commented May 15, 2023

Copy link
Copy Markdown
Member

Fixes #20262

Motivation

When there are hole messages, there is a bug in the API of skipping the message, please refer to the issue for the specific reason

Modifications

Remove recyclePositionRangeConverter, revert to normal loop.

Verifying this change

  • 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

Documentation

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

Matching PR in forked repository

PR in forked repository: (crossoverJie#8)

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label May 15, 2023
liangyepianzhou
liangyepianzhou approved these changes May 24, 2023
View reviewed changes

@liangyepianzhou liangyepianzhou left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

liangyepianzhou

This comment was marked as duplicate.

Sign in to view

@poorbarcode poorbarcode added this to the 3.1.0 milestone May 30, 2023
@liangyepianzhou

liangyepianzhou commented May 31, 2023

Copy link
Copy Markdown
Contributor

@poorbarcode @Technoboy- Please help take a look, thanks. 😸

@Technoboy- Technoboy- closed this Jun 1, 2023
@Technoboy- Technoboy- reopened this Jun 1, 2023
@codecov-commenter

codecov-commenter commented Jun 1, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.38%. Comparing base (5d5ec94) to head (2004bf8).
⚠️ Report is 2349 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #20326       +/-   ##
=============================================
+ Coverage     37.92%   72.38%   +34.45%     
- Complexity    12694    32022    +19328     
=============================================
  Files          1691     1867      +176     
  Lines        129047   143534    +14487     
  Branches      14070    16439     +2369     
=============================================
+ Hits          48942   103897    +54955     
+ Misses        73777    31250    -42527     
- Partials       6328     8387     +2059
Flag Coverage Δ
inttests 25.08% <ø> (+0.90%) ⬆️
systests 26.17% <ø> (+0.98%) ⬆️
unittests 71.36% <ø> (+37.90%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 77.60% <ø> (+28.44%) ⬆️

... and 1440 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Technoboy- Technoboy- merged commit c35b820 into apache:master Jun 1, 2023
Technoboy- pushed a commit that referenced this pull request Jun 1, 2023
@crossoverJie @Technoboy-
[fix][broker] Fix skip message API when hole messages exists (#20326)
e710742
lhotari
lhotari reviewed Jun 1, 2023
View reviewed changes
Comment thread managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
@@ -1779,7 +1779,6 @@ long getNumIndividualDeletedEntriesToSkip(long numEntries) {
} finally {
if (r.lowerEndpoint() instanceof PositionImplRecyclable) {
((PositionImplRecyclable) r.lowerEndpoint()).recycle();

@lhotari lhotari Jun 1, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@crossoverJie @liangyepianzhou I have a question about this change.

issue #20262 description contains "The reason for this issue is that the recycle() function reuses objects, causing the object referenced by r to change during runtime."

What is the reason that the lowerEndpoint is recycled and the upperEndpoint isn't?
why not just skip recycling completely? Omitting recycling completely would be fine if recycling is problematic.

@liangyepianzhou liangyepianzhou Jun 1, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We recycle an object after it is no longer used to save GC pause.
But the upperEndpoint is still used after being recycled, which makes this issue.
The lowerEndpoint is no longer used after being recycled, so we can recycle it.

state.startPosition = r.upperEndpoint();

lhotari pushed a commit that referenced this pull request Jun 6, 2023
@crossoverJie @lhotari
[fix][broker] Fix skip message API when hole messages exists (#20326)
1e45f83 
(cherry picked from commit c35b820)
lhotari pushed a commit that referenced this pull request Jun 6, 2023
@crossoverJie @lhotari
[fix][broker] Fix skip message API when hole messages exists (#20326)
3f84354 
(cherry picked from commit c35b820)
lhotari added a commit that referenced this pull request Jun 6, 2023
@lhotari
Backport test for #20326 to Java 8
6506d6a
lhotari pushed a commit that referenced this pull request Jun 6, 2023
@crossoverJie @lhotari
[fix][broker] Fix skip message API when hole messages exists (#20326)
967e1e1 
(cherry picked from commit c35b820)
lhotari pushed a commit that referenced this pull request Jun 6, 2023
@crossoverJie @lhotari
[fix][broker] Fix skip message API when hole messages exists (#20326)
168f50c 
(cherry picked from commit c35b820)
(cherry picked from commit 967e1e1)
@lhotari lhotari added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Jun 6, 2023
lhotari pushed a commit to datastax/pulsar that referenced this pull request Jun 6, 2023
@crossoverJie @lhotari
[fix][broker] Fix skip message API when hole messages exists (apache#…
039f9cc 
…20326)

(cherry picked from commit c35b820)
@michaeljmarshall

michaeljmarshall commented Jun 27, 2023

Copy link
Copy Markdown
Member

As discussed on the mailing list https://lists.apache.org/thread/w4jzk27qhtosgsz7l9bmhf1t7o9mxjhp, there is no plan to release 2.9.6, so I am going to remove the release/2.9.6 label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lhotari lhotari lhotari left review comments

@liangyepianzhou liangyepianzhou liangyepianzhou approved these changes

@Technoboy- Technoboy- Technoboy- approved these changes

@BewareMyPower BewareMyPower Awaiting requested review from BewareMyPower

@poorbarcode poorbarcode Awaiting requested review from poorbarcode

Assignees

@crossoverJie crossoverJie

Labels

area/broker cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 cherry-picked/branch-2.11 cherry-picked/branch-3.0 doc-not-needed Your PR changes do not impact docs release/2.8.5 release/2.10.5 release/2.11.2 release/3.0.1

Projects

None yet

Milestone

3.1.0

8 participants

@crossoverJie @liangyepianzhou @codecov-commenter @michaeljmarshall @lhotari @Technoboy- @RobertIndie @poorbarcode