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

[improve][broker] Get lowest PositionImpl from NavigableSet #18278

Merged

Conversation

lifepuzzlefun
Copy link
Contributor

@lifepuzzlefun lifepuzzlefun commented Nov 1, 2022

Fixes #18279

Motivation

change signature from Set<T> to NavigableSet<T>
which makes the caller to get lowest PositionImpl more efficient ( compare with stream().findfirst() )

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as (please describe tests).

Documentation

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

change signature from Set<T> to NavigableSet<T>
which makes the caller to get lowest PositionImpl more efficient.
@lifepuzzlefun lifepuzzlefun changed the title [cleanup][broker] Direct get lowest PositionImpl from TreeMap [cleanup][broker] Get lowest PositionImpl from NavigableSet Nov 1, 2022
@lifepuzzlefun
Copy link
Contributor Author

@codelipenghui @Jason918 please take a look, thank you~

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 1, 2022
@@ -285,7 +286,7 @@ public synchronized void readMoreEntries() {
}

havePendingReplayRead = true;
minReplayedPosition = messagesToReplayNow.stream().min(PositionImpl::compareTo).orElse(null);
minReplayedPosition = messagesToReplayNow.pollFirst();
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be first(), not pollFirst()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thanks for review.

@codelipenghui codelipenghui added ready-to-test type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/broker labels Nov 1, 2022
@codelipenghui codelipenghui changed the title [cleanup][broker] Get lowest PositionImpl from NavigableSet [improve][broker] Get lowest PositionImpl from NavigableSet Nov 1, 2022
@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

Copy link
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

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

Good!

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

@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2022

Codecov Report

Merging #18278 (552bcf6) into master (0866c3a) will increase coverage by 0.94%.
The diff coverage is 66.47%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18278      +/-   ##
============================================
+ Coverage     38.97%   39.92%   +0.94%     
- Complexity     8311     8591     +280     
============================================
  Files           683      687       +4     
  Lines         67325    67381      +56     
  Branches       7217     7217              
============================================
+ Hits          26239    26899     +660     
+ Misses        38079    37502     -577     
+ Partials       3007     2980      -27     
Flag Coverage Δ
unittests 39.92% <66.47%> (+0.94%) ⬆️

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

Impacted Files Coverage Δ
.../main/java/org/apache/pulsar/PulsarStandalone.java 0.00% <ø> (ø)
...pulsar/broker/service/PulsarCommandSenderImpl.java 70.15% <ø> (-4.21%) ⬇️
...apache/pulsar/broker/service/TopicListService.java 10.65% <0.00%> (-44.27%) ⬇️
...ervice/persistent/MessageRedeliveryController.java 56.09% <ø> (-4.88%) ⬇️
...ransaction/buffer/impl/InMemTransactionBuffer.java 0.00% <ø> (ø)
...nsaction/buffer/impl/TransactionBufferDisable.java 52.63% <ø> (ø)
.../metadata/v2/TransactionBufferSnapshotIndexes.java 0.00% <0.00%> (ø)
...a/v2/TransactionBufferSnapshotIndexesMetadata.java 0.00% <0.00%> (ø)
...oker/transaction/buffer/metadata/v2/TxnIDData.java 0.00% <0.00%> (ø)
...ulsar/utils/ConcurrentBitmapSortedLongPairSet.java 59.25% <ø> (ø)
... and 81 more

@lifepuzzlefun
Copy link
Contributor Author

/pulsarbot run-failure-checks

@nodece nodece merged commit 0c3c175 into apache:master Nov 2, 2022
Technoboy- pushed a commit to Technoboy-/pulsar that referenced this pull request May 9, 2023
…8278)

* [cleanup] Direct get lowest PositionImpl from TreeMap

change signature from Set<T> to NavigableSet<T>
which makes the caller to get lowest PositionImpl more efficient.

* change poll to first when call `NavigableSet`

* fix check style remove unused import

Co-authored-by: wangjinlong <[email protected]>
Technoboy- pushed a commit that referenced this pull request May 23, 2023
* [cleanup] Direct get lowest PositionImpl from TreeMap

change signature from Set<T> to NavigableSet<T>
which makes the caller to get lowest PositionImpl more efficient.

* change poll to first when call `NavigableSet`

* fix check style remove unused import

Co-authored-by: wangjinlong <[email protected]>
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jun 6, 2023
…8278)

* [cleanup] Direct get lowest PositionImpl from TreeMap

change signature from Set<T> to NavigableSet<T>
which makes the caller to get lowest PositionImpl more efficient.

* change poll to first when call `NavigableSet`

* fix check style remove unused import

Co-authored-by: wangjinlong <[email protected]>
(cherry picked from commit 01badd2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker cherry-picked/branch-2.10 cherry-picked/branch-2.11 doc-not-needed Your PR changes do not impact docs ready-to-test release/2.10.5 release/2.11.2 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[cleanup][broker] Direct get lowest PositionImpl from TreeMap
9 participants