[fix][broker] Compare batch index when accumulating acks and updating batchDeletedIndexes #18042

lordcheng10 · 2022-10-13T15:47:09Z

Motivation

When accumulating acks, update the batch index in batchDeletedIndexes without checking whether it is greater than the batch index of the previous ack：

pulsar/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java

Lines 1869 to 1872 in c732852

    
           if (config.isDeletionAtBatchIndexLevelEnabled() && batchDeletedIndexes != null) { 
        
               if (newPosition.ackSet != null) { 
        
                   batchDeletedIndexes.put(newPosition, BitSetRecyclable.create().resetWords(newPosition.ackSet)); 
        
                   newPosition = ledger.getPreviousPosition(newPosition);

This may cause the index of ack in the batch to fall back.

Should be modified to:

                lock.writeLock().lock();
                try {
                    BitSetRecyclable givenBitSet = BitSetRecyclable.create().resetWords(newPosition.ackSet);
                    batchDeletedIndexes.compute(newPosition,
                            (k, v) -> v == null || givenBitSet.nextSetBit(0) > v.nextSetBit(0)
                                    ? givenBitSet : v);
               } finally {
                    lock.writeLock().unlock();
                }

Documentation

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

Matching PR in forked repository

PR in forked repository: lordcheng10#28

lordcheng10 · 2022-10-14T15:52:18Z

@eolivelli @codelipenghui @Technoboy- @Jason918 @AnonHxy PTAL,thanks!

Jason918 · 2022-10-16T05:23:38Z

pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/BitSetRecyclable.java

@@ -105,6 +105,35 @@ private void recalculateWordsInUse() {
        wordsInUse = i+1; // The new logical size
    }

+    public int compareTo(BitSetRecyclable bitSetRecyclable) {
+        long[] words1 = toLongArray();


I suggest we avoid this memory copy in toLongArray, just use words and wordsInUse instead.

Fixed. @Jason918
PTAL,thanks!

remove compareTo method，use nextSetBit instead of compareTo！ @Jason918 PTAL,thanks!

codelipenghui · 2022-10-17T01:39:31Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java

-                batchDeletedIndexes.put(newPosition, BitSetRecyclable.create().resetWords(newPosition.ackSet));
+                lock.writeLock().lock();
+                try {
+                    setBatchDeletedIndexes(newPosition);


Looks like we just change to

BitSetRecyclable givenBitSet = BitSetRecyclable.create().resetWords(newPosition.ackSet); batchDeletedIndexes.compute(newPosition, (k, v) -> v == null || givenBitSet.nextClearBit(0) > v.nextClearBit(0) ? givenBitSet : v);

OK ,I will fix

Fixed, PTAL,thanks! @codelipenghui

nextClearBit is not suitable, because 0 means acked.
So use nextSetBit instead.

lordcheng10 · 2022-10-18T03:47:56Z

@Jason918 @codelipenghui PTAL,thanks!

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java

aloyszhang

LGTM

codecov-commenter · 2022-10-18T20:30:27Z

Codecov Report

Merging #18042 (3393572) into master (6c65ca0) will increase coverage by 11.94%.
The diff coverage is 59.90%.

@@              Coverage Diff              @@
##             master   #18042       +/-   ##
=============================================
+ Coverage     34.91%   46.85%   +11.94%     
- Complexity     5707    17892    +12185     
=============================================
  Files           607     1574      +967     
  Lines         53396   128353    +74957     
  Branches       5712    14123     +8411     
=============================================
+ Hits          18644    60144    +41500     
- Misses        32119    62010    +29891     
- Partials       2633     6199     +3566

Flag	Coverage Δ
unittests	`46.85% <59.90%> (+11.94%)`	⬆️

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

Impacted Files	Coverage Δ
...org/apache/pulsar/broker/ServiceConfiguration.java	`52.07% <ø> (ø)`
.../service/SystemTopicBasedTopicPoliciesService.java	`62.97% <0.00%> (+11.38%)`	⬆️
.../pulsar/broker/stats/BrokerOperabilityMetrics.java	`100.00% <ø> (+7.35%)`	⬆️
...va/org/apache/pulsar/client/impl/ConsumerBase.java	`21.95% <0.00%> (ø)`
...ache/pulsar/functions/utils/io/ConnectorUtils.java	`0.00% <0.00%> (ø)`
...nator/impl/MLTransactionMetadataStoreProvider.java	`0.00% <0.00%> (ø)`
.../org/apache/pulsar/broker/admin/v2/Namespaces.java	`55.32% <50.00%> (+47.30%)`	⬆️
...he/pulsar/functions/worker/rest/api/SinksImpl.java	`36.82% <50.00%> (ø)`
...apache/pulsar/proxy/server/DirectProxyHandler.java	`63.63% <50.00%> (ø)`
...ulsar/functions/worker/rest/api/ComponentImpl.java	`25.26% <57.14%> (ø)`
... and 1163 more

lordcheng10 · 2022-10-19T00:18:17Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-19T01:23:20Z

/pulsarbot run-failure-checks

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java

Jason918

LGTM

lordcheng10 · 2022-10-19T09:50:02Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-19T10:03:53Z

@Technoboy- PTAL,thanks!

lordcheng10 · 2022-10-19T14:54:14Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-20T00:42:50Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-20T01:53:24Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-20T01:54:45Z

@AnonHxy @Technoboy- PTAL,thanks!

… and check whether it is greater than the batch index of the previous ack

lordcheng10 · 2022-10-20T23:39:09Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-21T00:32:33Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-21T00:34:05Z

CI all passed: lordcheng10#28

lordcheng10 · 2022-10-21T00:37:59Z

@AnonHxy PTAL,thanks!

lordcheng10 · 2022-10-21T01:48:32Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-21T02:52:58Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-21T04:31:46Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-21T04:53:02Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-21T05:11:07Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-21T06:59:32Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-21T07:10:48Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-21T07:15:31Z

/pulsarbot run-failure-checks

… and check whether it is greater than the batch index of the previous ack (#18042) Co-authored-by: leolinchen <[email protected]>

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 13, 2022

lordcheng10 mentioned this pull request Oct 13, 2022

[fix][broker] Compare batch index when accumulating acks and updating batchDeletedIndexes lordcheng10/pulsar#28

Closed

lordcheng10 changed the title ~~[fix][broker] When accumulating acks, update the batch index in batchDeletedIndexes and check whether it is greater than the batch index of the previous ack~~ [fix][broker] Check the index size when accumulating acks and updating batchDeletedIndexes Oct 13, 2022

lordcheng10 changed the title ~~[fix][broker] Check the index size when accumulating acks and updating batchDeletedIndexes~~ [fix][broker] Compare batch index when accumulating acks and updating batchDeletedIndexes Oct 13, 2022

lordcheng10 changed the title ~~[fix][broker] Compare batch index when accumulating acks and updating batchDeletedIndexes~~ [improve][broker] Reduce unnecessary persistence of markdelete Oct 14, 2022

lordcheng10 mentioned this pull request Oct 14, 2022

[improve][broker] Reduce unnecessary persistence of markdelete #18006

Closed

4 tasks

lordcheng10 changed the title ~~[improve][broker] Reduce unnecessary persistence of markdelete~~ [fix][broker] Compare batch index when accumulating acks and updating batchDeletedIndexes Oct 14, 2022

lordcheng10 force-pushed the batch_index_update_check branch from 99e38d8 to 3567d01 Compare October 16, 2022 04:04

Jason918 reviewed Oct 16, 2022

View reviewed changes

lordcheng10 requested a review from Jason918 October 16, 2022 08:38

codelipenghui assigned lordcheng10 Oct 17, 2022

codelipenghui added this to the 2.12.0 milestone Oct 17, 2022

codelipenghui added type/bug The PR fixed a bug or issue reported a bug area/broker labels Oct 17, 2022

codelipenghui reviewed Oct 17, 2022

View reviewed changes

lordcheng10 force-pushed the batch_index_update_check branch from ff72a2c to 6f89d69 Compare October 17, 2022 05:45

lordcheng10 requested review from codelipenghui and removed request for Jason918 October 17, 2022 05:46

Jason918 reviewed Oct 18, 2022

View reviewed changes

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java Outdated Show resolved Hide resolved

aloyszhang approved these changes Oct 18, 2022

View reviewed changes

lordcheng10 force-pushed the batch_index_update_check branch from 227941e to 7d6a3fd Compare October 18, 2022 07:55

Jason918 reviewed Oct 19, 2022

View reviewed changes

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java Outdated Show resolved Hide resolved

Jason918 approved these changes Oct 19, 2022

View reviewed changes

When accumulating acks, update the batch index in batchDeletedIndexes…

6ad5478

… and check whether it is greater than the batch index of the previous ack

lordcheng10 force-pushed the batch_index_update_check branch from 3393572 to 6ad5478 Compare October 20, 2022 11:39

aloyszhang merged commit fa328a4 into apache:master Oct 21, 2022

Technoboy- modified the milestones: 2.12.0, 2.11.0 Oct 31, 2022

Technoboy- added the cherry-picked/branch-2.11 label Oct 31, 2022

Technoboy- pushed a commit that referenced this pull request Oct 31, 2022

When accumulating acks, update the batch index in batchDeletedIndexes…

043cb9f

… and check whether it is greater than the batch index of the previous ack (#18042) Co-authored-by: leolinchen <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][broker] Compare batch index when accumulating acks and updating batchDeletedIndexes #18042

[fix][broker] Compare batch index when accumulating acks and updating batchDeletedIndexes #18042

lordcheng10 commented Oct 13, 2022 •

edited

Loading

lordcheng10 commented Oct 14, 2022

Jason918 Oct 16, 2022

lordcheng10 Oct 16, 2022

lordcheng10 Oct 17, 2022

codelipenghui Oct 17, 2022

lordcheng10 Oct 17, 2022

lordcheng10 Oct 17, 2022

lordcheng10 Oct 17, 2022

lordcheng10 commented Oct 18, 2022

aloyszhang left a comment

codecov-commenter commented Oct 18, 2022 •

edited

Loading

lordcheng10 commented Oct 19, 2022

lordcheng10 commented Oct 19, 2022

Jason918 left a comment

lordcheng10 commented Oct 19, 2022

lordcheng10 commented Oct 19, 2022

lordcheng10 commented Oct 19, 2022

lordcheng10 commented Oct 20, 2022

lordcheng10 commented Oct 20, 2022

lordcheng10 commented Oct 20, 2022 •

edited

Loading

lordcheng10 commented Oct 20, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

	if (config.isDeletionAtBatchIndexLevelEnabled() && batchDeletedIndexes != null) {
	if (newPosition.ackSet != null) {
	batchDeletedIndexes.put(newPosition, BitSetRecyclable.create().resetWords(newPosition.ackSet));
	newPosition = ledger.getPreviousPosition(newPosition);

[fix][broker] Compare batch index when accumulating acks and updating batchDeletedIndexes #18042

[fix][broker] Compare batch index when accumulating acks and updating batchDeletedIndexes #18042

Conversation

lordcheng10 commented Oct 13, 2022 • edited Loading

Motivation

Documentation

Matching PR in forked repository

lordcheng10 commented Oct 14, 2022

Jason918 Oct 16, 2022

Choose a reason for hiding this comment

lordcheng10 Oct 16, 2022

Choose a reason for hiding this comment

lordcheng10 Oct 17, 2022

Choose a reason for hiding this comment

codelipenghui Oct 17, 2022

Choose a reason for hiding this comment

lordcheng10 Oct 17, 2022

Choose a reason for hiding this comment

lordcheng10 Oct 17, 2022

Choose a reason for hiding this comment

lordcheng10 Oct 17, 2022

Choose a reason for hiding this comment

lordcheng10 commented Oct 18, 2022

aloyszhang left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 18, 2022 • edited Loading

Codecov Report

lordcheng10 commented Oct 19, 2022

lordcheng10 commented Oct 19, 2022

Jason918 left a comment

Choose a reason for hiding this comment

lordcheng10 commented Oct 19, 2022

lordcheng10 commented Oct 19, 2022

lordcheng10 commented Oct 19, 2022

lordcheng10 commented Oct 20, 2022

lordcheng10 commented Oct 20, 2022

lordcheng10 commented Oct 20, 2022 • edited Loading

lordcheng10 commented Oct 20, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 21, 2022

lordcheng10 commented Oct 13, 2022 •

edited

Loading

codecov-commenter commented Oct 18, 2022 •

edited

Loading

lordcheng10 commented Oct 20, 2022 •

edited

Loading