Skip to content

[fix][broker] Fix return the earliest position when query position by timestamp.#20457

Merged
Jason918 merged 5 commits intoapache:masterfrom
hanmz:fix-opFindNewest
Jun 29, 2023
Merged

[fix][broker] Fix return the earliest position when query position by timestamp.#20457
Jason918 merged 5 commits intoapache:masterfrom
hanmz:fix-opFindNewest

Conversation

@hanmz
Copy link
Copy Markdown
Contributor

@hanmz hanmz commented Jun 1, 2023

Fixes #20368

Master Issue: #20368

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: hanmz#1

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 1, 2023
Copy link
Copy Markdown
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Please add a test case for this kind of behavior change. Also, add a link to your personal fork CI to prove that existing tests are not affected. See https://pulsar.apache.org/contribute/personal-ci/

Barely changing boolean logic is risky.

@hanmz hanmz changed the title Fix return the earliest position when query position by timestamp. [fix][broker] Fix return the earliest position when query position by timestamp. Jun 26, 2023
@hanmz hanmz requested a review from tisonkun June 26, 2023 12:15
@wolfstudy wolfstudy added the type/bug The PR fixed a bug or issue reported a bug label Jun 26, 2023
@hanmz hanmz removed their assignment Jun 26, 2023
Copy link
Copy Markdown
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

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

LGTM

@Jason918
Copy link
Copy Markdown
Contributor

/pulsarbot rerun-failure-checks

Copy link
Copy Markdown
Member

@wolfstudy wolfstudy left a comment

Choose a reason for hiding this comment

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

LGTM +1

Copy link
Copy Markdown
Contributor

@HQebupt HQebupt left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@AnonHxy AnonHxy left a comment

Choose a reason for hiding this comment

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

LGTM with minor comments

Copy link
Copy Markdown
Member

@StevenLuMT StevenLuMT left a comment

Choose a reason for hiding this comment

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

LGTM

@hanmz
Copy link
Copy Markdown
Contributor Author

hanmz commented Jun 28, 2023

/pulsarbot rerun-failure-checks

@hanmz
Copy link
Copy Markdown
Contributor Author

hanmz commented Jun 28, 2023

/pulsarbot rerun-failure-checks

@hanmz hanmz requested review from Jason918 and wolfstudy June 29, 2023 06:34
@hanmz
Copy link
Copy Markdown
Contributor Author

hanmz commented Jun 29, 2023

/pulsarbot rerun-failure-checks

@dave2wave
Copy link
Copy Markdown
Member

You broke the build by checking in an empty file

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.08%. Comparing base (2b01f83) to head (f1958ff).
⚠️ Report is 2374 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #20457       +/-   ##
=============================================
+ Coverage     33.58%   73.08%   +39.50%     
- Complexity    12127    32010    +19883     
=============================================
  Files          1613     1867      +254     
  Lines        126241   138758    +12517     
  Branches      13770    15263     +1493     
=============================================
+ Hits          42396   101416    +59020     
+ Misses        78331    29298    -49033     
- Partials       5514     8044     +2530     
Flag Coverage Δ
inttests 24.13% <0.00%> (-0.06%) ⬇️
systests 25.09% <0.00%> (?)
unittests 72.35% <100.00%> (+40.31%) ⬆️

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

Files with missing lines Coverage Δ
...a/org/apache/bookkeeper/mledger/ManagedCursor.java 42.85% <ø> (ø)
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 78.98% <100.00%> (+32.78%) ⬆️
...er/service/persistent/PersistentMessageFinder.java 65.90% <ø> (+11.36%) ⬆️

... and 1519 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.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Return the earliest position when query position by timestamp.