[fix] [broker] Msg delivery is stuck due to items in the collection recentlyJoinedConsumers are out-of-order #23795

poorbarcode · 2024-12-31T05:04:09Z

Motivation

Version: 3.0.7, thanks for @shibd investigating the issue together and mentioning the key clue that items are out of order.

Issue 1: concurrentcy access a un-thread-safety collection

The variable recentlyJoinedConsumers of PersistentStickyKeyDispatcherMultipleConsumers is used to guarantee the delivery order of Key_Shared subscriptions, which is not a thread-safety collection.
Almost all operations that access this collection are in a lock block except in one place that the current PR fixes

Issue 2: the items in recentlyJoinedConsumers are out of order

First node 104142:122103
...
middle node 1 104142:122103
middle node 2 104046:283984
...
tail node 104046:284275

But we have more than one place that relies on the order rule, such as follows

https://github.com/apache/pulsar/blob/branch-3.0/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java#L377

        if (readType == ReadType.Replay) {
            // broker assumes the first node is the eldest one
            PositionImpl minReadPositionForRecentJoinedConsumer = recentlyJoinedConsumers.values().iterator().next();
            if (minReadPositionForRecentJoinedConsumer != null
                    && minReadPositionForRecentJoinedConsumer.compareTo(maxReadPosition) < 0) {
                maxReadPosition = minReadPositionForRecentJoinedConsumer;
            }
        }

https://github.com/apache/pulsar/blob/branch-3.0/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java#L428

            PositionImpl nextPositionOfTheMarkDeletePosition =
                    ((ManagedLedgerImpl) cursor.getManagedLedger()).getNextValidPosition(mdp);
            while (itr.hasNext()) {
                Map.Entry<Consumer, PositionImpl> entry = itr.next();
                if (entry.getValue().compareTo(nextPositionOfTheMarkDeletePosition) <= 0) {
                    itr.remove();
                    hasConsumerRemovedFromTheRecentJoinedConsumers = true;
                } else {
                    // break if one node does not match the check
                    break;
                }
            }

topic-stats.txt

 "consumersAfterMarkDeletePosition" : {
        "rudder-src-router-19-2" : "104142:122103",
        "rudder-src-router-22-3" : "104142:122103",
        "rudder-src-router-22-2" : "104142:122103",
        "rudder-src-router-12-3" : "104142:122103",
        "rudder-src-router-2-0" : "104142:122103",
        "rudder-src-router-4-1" : "104142:122103",
        "rudder-src-router-18-3" : "104142:122103",
        "rudder-src-router-29-2" : "104046:283984",
        "rudder-src-router-13-1" : "104046:283984",
        "rudder-src-router-13-0" : "104046:283984",
        "rudder-src-router-21-0" : "104046:283984",
        "rudder-src-router-21-1" : "104046:283984",
        "rudder-src-router-17-1" : "104046:283984",
        "rudder-src-router-1-1" : "104046:283984",
        "rudder-src-router-24-1" : "104046:283990",
        "rudder-src-router-3-1" : "104046:283990",
        "rudder-src-router-3-3" : "104046:283990",
        "rudder-src-router-13-2" : "104046:283990",
        "rudder-src-router-7-1" : "104046:283990",
        "rudder-src-router-7-3" : "104046:283990",
        "rudder-src-router-12-2" : "104046:283990",
        "rudder-src-router-2-1" : "104046:284068",
        "rudder-src-router-17-2" : "104046:284068",
        "rudder-src-router-17-0" : "104046:284068",
        "rudder-src-router-6-0" : "104046:284123",
        "rudder-src-router-1-2" : "104046:284123",
        "rudder-src-router-1-0" : "104046:284123",
        "rudder-src-router-18-0" : "104046:284123",
        "rudder-src-router-0-0" : "104046:284123",
        "rudder-src-router-16-0" : "104046:284123",
        "rudder-src-router-27-3" : "104046:284123",
        "rudder-src-router-15-1" : "104046:284124",
        "rudder-src-router-27-2" : "104046:284124",
        "rudder-src-router-15-2" : "104046:284124",
        "rudder-src-router-22-1" : "104046:284138",
        "rudder-src-router-12-0" : "104046:284138",
        "rudder-src-router-24-3" : "104046:284150",
        "rudder-src-router-4-0" : "104046:284150",
        "rudder-src-router-28-0" : "104046:284150",
        "rudder-src-router-28-1" : "104046:284150",
        "rudder-src-router-23-2" : "104046:284150",
        "rudder-src-router-6-1" : "104046:284154",
        "rudder-src-router-5-1" : "104046:284154",
        "rudder-src-router-6-3" : "104046:284154",
        "rudder-src-router-6-2" : "104046:284157",
        "rudder-src-router-5-2" : "104046:284157",
        "rudder-src-router-5-0" : "104046:284157",
        "rudder-src-router-5-3" : "104046:284161",
        "rudder-src-router-0-3" : "104046:284164",
        "rudder-src-router-28-3" : "104046:284164",
        "rudder-src-router-26-3" : "104046:284164",
        "rudder-src-router-11-3" : "104046:284164",
        "rudder-src-router-14-2" : "104046:284169",
        "rudder-src-router-4-2" : "104046:284177",
        "rudder-src-router-29-1" : "104046:284177",
        "rudder-src-router-2-3" : "104046:284177",
        "rudder-src-router-22-0" : "104046:284177",
        "rudder-src-router-23-1" : "104046:284177",
        "rudder-src-router-20-0" : "104046:284182",
        "rudder-src-router-10-2" : "104046:284182",
        "rudder-src-router-20-2" : "104046:284182",
        "rudder-src-router-25-3" : "104046:284182",
        "rudder-src-router-8-3" : "104046:284182",
        "rudder-src-router-8-2" : "104046:284182",
        "rudder-src-router-26-0" : "104046:284182",
        "rudder-src-router-26-2" : "104046:284182",
        "rudder-src-router-4-3" : "104046:284182",
        "rudder-src-router-9-1" : "104046:284182",
        "rudder-src-router-18-1" : "104046:284182",
        "rudder-src-router-0-2" : "104046:284182",
        "rudder-src-router-21-2" : "104046:284182",
        "rudder-src-router-7-2" : "104046:284193",
        "rudder-src-router-27-0" : "104046:284193",
        "rudder-src-router-27-1" : "104046:284193",
        "rudder-src-router-16-3" : "104046:284193",
        "rudder-src-router-21-3" : "104046:284193",
        "rudder-src-router-8-1" : "104046:284193",
        "rudder-src-router-10-1" : "104046:284193",
        "rudder-src-router-10-3" : "104046:284193",
        "rudder-src-router-15-0" : "104046:284193",
        "rudder-src-router-11-1" : "104046:284193",
        "rudder-src-router-11-2" : "104046:284193",
        "rudder-src-router-17-3" : "104046:284193",
        "rudder-src-router-25-0" : "104046:284193",
        "rudder-src-router-19-1" : "104046:284200",
        "rudder-src-router-24-0" : "104046:284200",
        "rudder-src-router-20-1" : "104046:284200",
        "rudder-src-router-14-0" : "104046:284200",
        "rudder-src-router-9-3" : "104046:284200",
        "rudder-src-router-8-0" : "104046:284204",
        "rudder-src-router-14-3" : "104046:284206",
        "rudder-src-router-9-0" : "104046:284206",
        "rudder-src-router-23-0" : "104046:284206",
        "rudder-src-router-23-3" : "104046:284211",
        "rudder-src-router-19-0" : "104046:284211",
        "rudder-src-router-3-0" : "104046:284221",
        "rudder-src-router-3-2" : "104046:284221",
        "rudder-src-router-14-1" : "104046:284223",
        "rudder-src-router-9-2" : "104046:284223",
        "rudder-src-router-29-0" : "104046:284223",
        "rudder-src-router-25-1" : "104046:284223",
        "rudder-src-router-25-2" : "104046:284223",
        "rudder-src-router-13-3" : "104046:284247",
        "rudder-src-router-10-0" : "104046:284251",
        "rudder-src-router-20-3" : "104046:284259",
        "rudder-src-router-18-2" : "104046:284266",
        "rudder-src-router-28-2" : "104046:284266",
        "rudder-src-router-26-1" : "104046:284266",
        "rudder-src-router-0-1" : "104046:284266",
        "rudder-src-router-7-0" : "104046:284266",
        "rudder-src-router-2-2" : "104046:284266",
        "rudder-src-router-15-3" : "104046:284268",
        "rudder-src-router-11-0" : "104046:284268",
        "rudder-src-router-19-3" : "104046:284268",
        "rudder-src-router-1-3" : "104046:284268",
        "rudder-src-router-24-2" : "104046:284275",
        "rudder-src-router-12-1" : "104046:284275",
        "rudder-src-router-16-1" : "104046:284275",
        "rudder-src-router-16-2" : "104046:284275"
}

Modifications

fix the concurrency issue
sort recentlyJoinedConsumers when adding/removing consumers, I have not found the root cause of items in recentlyJoinedConsumers being out of order, I will continue working on it. Before that let us fix the issue fast

Documentation

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

Matching PR in forked repository

PR in forked repository: x

… un-thread-safety collection recentlyJoinedConsumers

poorbarcode · 2024-12-31T07:33:21Z

/pulsarbot rerun-failure-checks

… un-thread-safety collection recentlyJoinedConsumers

.../pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumersClassic.java

codecov-commenter · 2025-01-02T04:49:42Z

Codecov Report

Attention: Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.14%. Comparing base (bbc6224) to head (5a190f4).
Report is 824 commits behind head on master.

Files with missing lines	Patch %	Lines
...ntStickyKeyDispatcherMultipleConsumersClassic.java	96.29%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #23795      +/-   ##
============================================
+ Coverage     73.57%   74.14%   +0.56%     
+ Complexity    32624    31761     -863     
============================================
  Files          1877     1853      -24     
  Lines        139502   143412    +3910     
  Branches      15299    16286     +987     
============================================
+ Hits         102638   106326    +3688     
+ Misses        28908    28711     -197     
- Partials       7956     8375     +419

Flag	Coverage Δ
inttests	`26.70% <0.00%> (+2.12%)`	⬆️
systests	`23.18% <0.00%> (-1.14%)`	⬇️
unittests	`73.64% <96.29%> (+0.79%)`	⬆️

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

Files with missing lines	Coverage Δ
...ntStickyKeyDispatcherMultipleConsumersClassic.java	`88.04% <96.29%> (ø)`

... and 1026 files with indirect coverage changes

lhotari

LGTM

…nsumers are out-of-order (#23795) (cherry picked from commit 4a01423)

…centlyJoinedConsumers are out-of-order (#23795) (cherry picked from commit 4a01423)

…nsumers are out-of-order (apache#23795) (cherry picked from commit 4a01423) (cherry picked from commit ca535a2)

[fix] [broker] Msg delivery is stuck due to concurrency modifying the…

48f7506

… un-thread-safety collection recentlyJoinedConsumers

poorbarcode added release/3.0.9 release/3.3.4 release/4.0.2 labels Dec 31, 2024

poorbarcode requested review from lhotari, Technoboy-, codelipenghui, gaoran10 and shibd December 31, 2024 05:04

poorbarcode self-assigned this Dec 31, 2024

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 31, 2024

poorbarcode added the type/bug The PR fixed a bug or issue reported a bug label Dec 31, 2024

dao-jun approved these changes Dec 31, 2024

View reviewed changes

poorbarcode added the ready-to-test label Dec 31, 2024

poorbarcode changed the title ~~[fix] [broker] Msg delivery is stuck due to concurrency modifying the un-thread-safety collection recentlyJoinedConsumers~~ [fix] [broker] Msg topic stats failed due to concurrency modifying&reading the un-thread-safety collection recentlyJoinedConsumers Dec 31, 2024

poorbarcode added 2 commits December 31, 2024 16:40

[fix] [broker] Msg delivery is stuck due to concurrency modifying the…

ead3a51

… un-thread-safety collection recentlyJoinedConsumers

[fix] [broker] Msg delivery is stuck due to concurrency modifying the…

46f2d79

… un-thread-safety collection recentlyJoinedConsumers

poorbarcode changed the title ~~[fix] [broker] Msg topic stats failed due to concurrency modifying&reading the un-thread-safety collection recentlyJoinedConsumers~~ [fix] [broker] Msg delivery is stuck due to items in the collection recentlyJoinedConsumers are out-of-order Dec 31, 2024

shibd approved these changes Dec 31, 2024

View reviewed changes

checkstyle

dfbb11f

lhotari reviewed Dec 31, 2024

View reviewed changes

.../pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumersClassic.java Outdated Show resolved Hide resolved

lhotari reviewed Dec 31, 2024

View reviewed changes

.../pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumersClassic.java Outdated Show resolved Hide resolved

address comment

6ef7d25

poorbarcode requested a review from lhotari January 1, 2025 12:48

fix tests

5a190f4

Technoboy- reviewed Jan 2, 2025

View reviewed changes

.../pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumersClassic.java Show resolved Hide resolved

poorbarcode requested a review from Technoboy- January 2, 2025 02:03

gaoran10 approved these changes Jan 2, 2025

View reviewed changes

lhotari approved these changes Jan 2, 2025

View reviewed changes

lhotari merged commit 4a01423 into apache:master Jan 2, 2025
52 checks passed

lhotari pushed a commit that referenced this pull request Jan 2, 2025

Msg delivery is stuck due to items in the collection recentlyJoinedCo…

ca535a2

…nsumers are out-of-order (#23795) (cherry picked from commit 4a01423)

lhotari added the cherry-picked/branch-3.0 label Jan 2, 2025

lhotari pushed a commit that referenced this pull request Jan 2, 2025

Msg delivery is stuck due to items in the collection recentlyJoinedCo…

b0df8d1

…nsumers are out-of-order (#23795) (cherry picked from commit 4a01423)

lhotari added the cherry-picked/branch-3.3 label Jan 2, 2025

lhotari pushed a commit that referenced this pull request Jan 2, 2025

[fix][broker] Msg delivery is stuck due to items in the collection re…

c304bf5

…centlyJoinedConsumers are out-of-order (#23795) (cherry picked from commit 4a01423)

lhotari added the cherry-picked/branch-4.0 label Jan 2, 2025

nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jan 3, 2025

Msg delivery is stuck due to items in the collection recentlyJoinedCo…

0dfa4f2

…nsumers are out-of-order (apache#23795) (cherry picked from commit 4a01423) (cherry picked from commit ca535a2)

srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jan 3, 2025

Msg delivery is stuck due to items in the collection recentlyJoinedCo…

b22764f

…nsumers are out-of-order (apache#23795) (cherry picked from commit 4a01423) (cherry picked from commit ca535a2)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] [broker] Msg delivery is stuck due to items in the collection recentlyJoinedConsumers are out-of-order #23795

[fix] [broker] Msg delivery is stuck due to items in the collection recentlyJoinedConsumers are out-of-order #23795

poorbarcode commented Dec 31, 2024 •

edited

Loading

poorbarcode commented Dec 31, 2024

codecov-commenter commented Jan 2, 2025 •

edited

Loading

lhotari left a comment

[fix] [broker] Msg delivery is stuck due to items in the collection recentlyJoinedConsumers are out-of-order #23795

[fix] [broker] Msg delivery is stuck due to items in the collection recentlyJoinedConsumers are out-of-order #23795

Conversation

poorbarcode commented Dec 31, 2024 • edited Loading

Motivation

Modifications

Documentation

Matching PR in forked repository

poorbarcode commented Dec 31, 2024

codecov-commenter commented Jan 2, 2025 • edited Loading

Codecov Report

lhotari left a comment

Choose a reason for hiding this comment

poorbarcode commented Dec 31, 2024 •

edited

Loading

codecov-commenter commented Jan 2, 2025 •

edited

Loading