Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jun 1, 2023

Conversation

crossoverJie
Copy link
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
Copy link
Contributor

@liangyepianzhou liangyepianzhou left a comment

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.

@liangyepianzhou
Copy link
Contributor

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

@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2023

Codecov Report

Merging #20326 (2004bf8) into master (5d5ec94) will increase coverage by 34.45%.
The diff coverage is n/a.

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.

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

... and 1440 files with indirect coverage changes

@@ -1779,7 +1779,6 @@ long getNumIndividualDeletedEntriesToSkip(long numEntries) {
} finally {
if (r.lowerEndpoint() instanceof PositionImplRecyclable) {
((PositionImplRecyclable) r.lowerEndpoint()).recycle();
Copy link
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.

Copy link
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
lhotari pushed a commit that referenced this pull request Jun 6, 2023
lhotari added a commit that referenced this pull request Jun 6, 2023
lhotari pushed a commit that referenced this pull request Jun 6, 2023
lhotari pushed a commit that referenced this pull request Jun 6, 2023
(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
@michaeljmarshall
Copy link
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
Projects
None yet
8 participants