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 wrong isEmpty method of ConcurrentOpenLongPairRangeSet #12953

Conversation

BewareMyPower
Copy link
Contributor

Motivation

The ConcurrentOpenLongPairRangeSet#isEmpty implementation is wrong. See

        AtomicBoolean isEmpty = new AtomicBoolean(false);
        rangeBitSetMap.forEach((key, val) -> {
            if (!isEmpty.get()) { // [1] isEmpty is always false, so this condition always meets
                return;
            isEmpty.set(val.isEmpty()); // it never reaches here
        });
        return isEmpty.get(); // [2] So if rangeBitSetMap is not empty, isEmpty() always return true

The comment of [1] should be isEmpty.get(), not !isEmpty.get(). However, the implementation is still bad because forEach method will iterate over the whole map. If the map has many entries, the performance cost will not be ignorable.

Modifications

  • Fix ConcurrentOpenLongPairRangeSet#isEmpty. Instead of calling forEach, here we use a trivial for loop and break the loop if there is an entry whose value is a non-empty set.
  • Add a test (testIsEmpty), which adds an empty set to ConcurrentOpenLongPairRangeSet. Before this patch, the isEmpty() returns false, which is incorrect.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added test ConcurrentOpenLongPairRangeSetTest#testIsEmpty.

@BewareMyPower BewareMyPower added type/bug The PR fixed a bug or issue reported a bug doc-not-needed Your PR changes do not impact docs release/2.9.1 release/2.8.3 labels Nov 24, 2021
@BewareMyPower BewareMyPower self-assigned this Nov 24, 2021
Copy link
Member

@michaeljmarshall michaeljmarshall 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
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 codelipenghui added this to the 2.10.0 milestone Nov 25, 2021
@codelipenghui codelipenghui merged commit 6cc5cff into apache:master Nov 25, 2021
@BewareMyPower BewareMyPower deleted the bewaremypower/fix-range-set-is-empty branch November 25, 2021 04:05
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Nov 25, 2021
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Nov 25, 2021
codelipenghui pushed a commit that referenced this pull request Nov 26, 2021
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Nov 26, 2021
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request Nov 29, 2021
fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021
codelipenghui pushed a commit that referenced this pull request Dec 21, 2021
@codelipenghui codelipenghui added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Dec 21, 2021
gaoran10 pushed a commit that referenced this pull request Jan 18, 2022
### Motivation
There are some methods implemented with an inefficient forEach loop, so fix it to get better performance. It is similar to #12953

### Modifications

I rewrite it with the `for` loop.
codelipenghui pushed a commit that referenced this pull request Jan 18, 2022
### Motivation
There are some methods implemented with an inefficient forEach loop, so fix it to get better performance. It is similar to #12953

### Modifications

I rewrite it with the `for` loop.

(cherry picked from commit 9c94cd7)
codelipenghui pushed a commit that referenced this pull request Jan 18, 2022
### Motivation
There are some methods implemented with an inefficient forEach loop, so fix it to get better performance. It is similar to #12953

### Modifications

I rewrite it with the `for` loop.

(cherry picked from commit 9c94cd7)
Shawyeok pushed a commit to Shawyeok/pulsar that referenced this pull request Sep 6, 2022
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.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.8.3 release/2.9.2 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants