[fix][broker] Fix `RoaringBitmap.contains` can't check value 65535 #20176

coderzc · 2023-04-25T03:06:46Z

Motivation

MessageRedeliveryController uses the RoaringBitmap under the covers to track entryIds to redeliver.
The bug in RoaringBitmap (that has now been fixed) caused at least the case that the bit index 65535 couldn't be set.
The consequence of this would be that a message that should be redelivered doesn't get delivered and this could get Key_Shared subscriptions to get stuck. This could impact also other subscription types and cause a backlog to build up.
This bug was introduced in 2.10.2 with #17804 since that's when MessageRedeliveryController switched to use the RoaringBitmap based datastructure.

Modifications

Upgrade RoaringBitmap version to 0.9.44, this issue be fixed by PR (Verifying and fixing issue 623 RoaringBitmap/RoaringBitmap#624)
Add test for RoaringBitmap.contains

Verifying this change

Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

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

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end deployment with large payloads (10MB)
Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

Documentation

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

Matching PR in forked repository

PR in forked repository:

codelipenghui · 2023-04-25T05:47:42Z

Interesting, ConcurrentBitmapSortedLongPairSet also used the same method.

coderzc · 2023-04-25T05:54:33Z

Interesting, ConcurrentBitmapSortedLongPairSet also used the same method.

Yeah, I'm not sure if our usage is wrong

This reverts commit f6ff70a.

codecov-commenter · 2023-05-08T08:00:44Z

Codecov Report

Merging #20176 (e28b882) into master (35e9897) will increase coverage by 35.24%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             master   #20176       +/-   ##
=============================================
+ Coverage     37.64%   72.89%   +35.24%     
- Complexity    12522    31963    +19441     
=============================================
  Files          1691     1868      +177     
  Lines        128933   138594     +9661     
  Branches      14061    15246     +1185     
=============================================
+ Hits          48540   101026    +52486     
+ Misses        74070    29531    -44539     
- Partials       6323     8037     +1714

Flag	Coverage Δ
inttests	`24.14% <ø> (-0.02%)`	⬇️
systests	`24.77% <ø> (-0.10%)`	⬇️
unittests	`72.20% <ø> (+39.09%)`	⬆️

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

see 1431 files with indirect coverage changes

tisonkun

Good to go. One comment that can be follow-up insight.

tisonkun · 2023-05-08T08:21:36Z

pulsar-broker/src/test/java/org/apache/pulsar/RoaringbitmapTest.java

+        }
+
+        for (long i = 1; i <= 100_000; i++) {
+            assertTrue(roaringBitmap.contains(i, i + 1));


nit: add description so that we know which i fails instead of generic:

java.lang.AssertionError: Expected :true Actual :false

tisonkun

Good to go. One comment that can be follow-up insight.

tisonkun · 2023-05-08T08:23:23Z

Merging...

…20176) (cherry picked from commit 2f9f5df)

…pache#20176) (cherry picked from commit 2f9f5df) (cherry picked from commit edc3a6f)

lhotari · 2023-05-17T05:59:52Z

@coderzc Please update the PR description with the impact of this bug.

Here's my version of describing the impact:
MessageRedeliveryController uses the RoaringBitmap under the covers to track entryIds to redeliver.
The bug in RoaringBitmap (that has now been fixed) caused at least the case that the bit index 65535 couldn't be set.
The consequence of this would be that a message that should be redelivered doesn't get delivered and this could get Key_Shared subscriptions to get stuck. This could impact also other subscription types and cause a backlog to build up.
This bug was introduced in 2.10.2 with #17804 since that's when MessageRedeliveryController switched to use the RoaringBitmap based datastructure.

@codelipenghui @coderzc Is this the correct interpretation?

codelipenghui · 2023-05-17T06:03:30Z

@lhotari Yes, looks good to me. Thanks.

michaeljmarshall · 2023-06-27T21:55:52Z

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

Fix RoaringBitmap contains can't check value 65535

f6ff70a

coderzc requested review from mattisonchao and codelipenghui April 25, 2023 03:06

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 25, 2023

coderzc changed the title ~~[fix][broker] Fix RoaringBitmap contains can't check value 65535~~ [fix][broker] Fix RoaringBitmap.contains can't check value 65535 Apr 25, 2023

coderzc added type/bug The PR fixed a bug or issue reported a bug area/broker labels Apr 25, 2023

coderzc added 2 commits May 7, 2023 20:45

Upgrade roaringbitmap version to 0.9.44

002f0c7

Revert "Fix RoaringBitmap contains can't check value 65535"

e28b882

This reverts commit f6ff70a.

coderzc added release/3.0.1 release/2.11.2 labels May 7, 2023

coderzc added this to the 3.1.0 milestone May 7, 2023

coderzc self-assigned this May 7, 2023

coderzc added release/2.10.5 ready-to-test labels May 7, 2023

mattisonchao approved these changes May 8, 2023

View reviewed changes

coderzc closed this May 8, 2023

coderzc reopened this May 8, 2023

poorbarcode approved these changes May 8, 2023

View reviewed changes

coderzc added the release/2.9.6 label May 8, 2023

tisonkun approved these changes May 8, 2023

View reviewed changes

tisonkun merged commit 2f9f5df into apache:master May 8, 2023

poorbarcode pushed a commit that referenced this pull request May 8, 2023

[fix][broker] Fix RoaringBitmap.contains can't check value 65535 (#…

c8ca912

…20176) (cherry picked from commit 2f9f5df)

poorbarcode added the cherry-picked/branch-3.0 label May 8, 2023

poorbarcode pushed a commit that referenced this pull request May 8, 2023

[fix][broker] Fix RoaringBitmap.contains can't check value 65535 (#…

43bf682

…20176) (cherry picked from commit 2f9f5df)

poorbarcode pushed a commit that referenced this pull request May 8, 2023

[fix][broker] Fix RoaringBitmap.contains can't check value 65535 (#…

edc3a6f

…20176) (cherry picked from commit 2f9f5df)

poorbarcode added cherry-picked/branch-2.10 cherry-picked/branch-2.11 labels May 8, 2023

coderzc added a commit that referenced this pull request May 8, 2023

[fix][broker] Fix RoaringBitmap.contains can't check value 65535 (#…

283e8b7

…20176) (cherry picked from commit 2f9f5df)

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request May 11, 2023

[fix][broker] Fix RoaringBitmap.contains can't check value 65535 (a…

e0065b5

…pache#20176) (cherry picked from commit 2f9f5df) (cherry picked from commit edc3a6f)

coderzc added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label May 31, 2023

michaeljmarshall removed the release/2.9.6 label Jun 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][broker] Fix `RoaringBitmap.contains` can't check value 65535 #20176

[fix][broker] Fix `RoaringBitmap.contains` can't check value 65535 #20176

coderzc commented Apr 25, 2023 •

edited

Loading

codelipenghui commented Apr 25, 2023

coderzc commented Apr 25, 2023 •

edited

Loading

codecov-commenter commented May 8, 2023

tisonkun left a comment

tisonkun May 8, 2023 •

edited

Loading

tisonkun left a comment

tisonkun commented May 8, 2023

lhotari commented May 17, 2023

codelipenghui commented May 17, 2023

michaeljmarshall commented Jun 27, 2023

[fix][broker] Fix RoaringBitmap.contains can't check value 65535 #20176

[fix][broker] Fix RoaringBitmap.contains can't check value 65535 #20176

Conversation

coderzc commented Apr 25, 2023 • edited Loading

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Matching PR in forked repository

codelipenghui commented Apr 25, 2023

coderzc commented Apr 25, 2023 • edited Loading

codecov-commenter commented May 8, 2023

Codecov Report

tisonkun left a comment

Choose a reason for hiding this comment

tisonkun May 8, 2023 • edited Loading

Choose a reason for hiding this comment

tisonkun left a comment

Choose a reason for hiding this comment

tisonkun commented May 8, 2023

lhotari commented May 17, 2023

codelipenghui commented May 17, 2023

michaeljmarshall commented Jun 27, 2023

[fix][broker] Fix `RoaringBitmap.contains` can't check value 65535 #20176

[fix][broker] Fix `RoaringBitmap.contains` can't check value 65535 #20176

coderzc commented Apr 25, 2023 •

edited

Loading

coderzc commented Apr 25, 2023 •

edited

Loading

tisonkun May 8, 2023 •

edited

Loading