[fix] [broker] Make the new exclusive consumer instead the inactive one faster #21183

poorbarcode · 2023-09-14T09:55:23Z

Motivation

There is an issue similar to the #21155 fixed one.

The client assumed the connection was inactive, but the Broker assumed the connection was fine. The Client tried to use a new connection to reconnect an exclusive consumer, then got an error Exclusive consumer is already connected

Modifications

Check the connection of the old consumer is available when the new one tries to subscribe

Documentation

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

Matching PR in forked repository

PR in forked repository: x

Dismiss

codelipenghui · 2023-09-15T13:18:52Z

...r/src/main/java/org/apache/pulsar/broker/service/AbstractDispatcherSingleActiveConsumer.java

+            Consumer actConsumer = ACTIVE_CONSUMER_UPDATER.get(this);
+            if (actConsumer != null) {
+                actConsumer.cnx().checkConnectionLiveness();
+            }


But the new consumer will not retry to connect to the topic, right? Do we need to wait for the connection liveness check done?

@codelipenghui

But the new consumer will not retry to connect to the topic, right? Do we need to wait for the connection liveness check to be done?

Sure, I did this improve.

(Highlight) I have a concern:

Background: the PR #20026 changed the method dispatcher.addConsumer to an asynchronous method, it broke the lock of synchronized(dispathcer.this), this change only affected the releases larger than 3.0.0.

Concern: The improvement "wait for the connection liveness check done" relies on the asynchronous method dispatcher.addConsumer. I am thinking about whether to accept the patch #20026 and fix the broken lock(this would make the logic complex), or revert this patch to make the logic simple.

I'd like to know your advice on the concern.

After talking with @codelipenghui @BewareMyPower @RobertIndie @gaoran10 @Technoboy- , I will write a new PR to fix the lock that broke by the PR #20026

...r/src/main/java/org/apache/pulsar/broker/service/AbstractDispatcherSingleActiveConsumer.java

poorbarcode · 2023-10-11T03:45:54Z

rebase master

pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java

BewareMyPower

The client assumed the connection was inactive, but the Broker assumed the connection was fine.

In addition, could you explain in which case could the case described in the PR happen?

BewareMyPower · 2023-10-16T14:35:12Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java

+        pulsar.getConfig().setConnectionLivenessCheckTimeoutMillis(5000);
+    }
+
+    private AtomicBoolean startChannelMonitorToHandleUserTask() {


This method is hard to understand. Maybe it's better to add some comments.

I removed AtomicBoolean channel1MonitorStopped = startChannelMonitorToHandleUserTask(); and channel1MonitorStopped.set(true); and the tests still passed.

The channel is typed EmbeddedChannel. Once we call channel.execute(runnable), there is no background thread to run it.

So starting a background thread to trigger the tasks in the queue will make the test more stable

Added a comment for this method

I removed AtomicBoolean channel1MonitorStopped = startChannelMonitorToHandleUserTask(); and channel1MonitorStopped.set(true); and the tests still passed.

If you run the test testHandleConsumerAfterClientChannelInactive, It has a high probability of failure.

If you run the test testHandleConsumerAfterClientChannelInactiveWhenDisabledFeatureConnectionLivenessCheckTimeoutMillis, it will always be passed, because it just confirms that this fix will not be affected if disabled the feature connectionLivenessCheckTimeoutMillis

pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java

…ne quickly

…subscribe

...r/src/main/java/org/apache/pulsar/broker/service/AbstractDispatcherSingleActiveConsumer.java

pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java

…ne faster (#21183) ### Motivation There is an issue similar to the #21155 fixed one. The client assumed the connection was inactive, but the Broker assumed the connection was fine. The Client tried to use a new connection to reconnect an exclusive consumer, then got an error `Exclusive consumer is already connected` ### Modifications - Check the connection of the old consumer is available when the new one tries to subscribe (cherry picked from commit 29db8f8)

…ne faster (#21183) There is an issue similar to the #21155 fixed one. The client assumed the connection was inactive, but the Broker assumed the connection was fine. The Client tried to use a new connection to reconnect an exclusive consumer, then got an error `Exclusive consumer is already connected` - Check the connection of the old consumer is available when the new one tries to subscribe (cherry picked from commit 29db8f8)

…ne faster (apache#21183) There is an issue similar to the apache#21155 fixed one. The client assumed the connection was inactive, but the Broker assumed the connection was fine. The Client tried to use a new connection to reconnect an exclusive consumer, then got an error `Exclusive consumer is already connected` - Check the connection of the old consumer is available when the new one tries to subscribe (cherry picked from commit 29db8f8) (cherry picked from commit b796f56)

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

poorbarcode self-assigned this Sep 14, 2023

poorbarcode added release/3.0.2 release/2.11.3 release/2.10.6 category/reliability The function does not work properly in certain specific environments or failures. e.g. data lost labels Sep 14, 2023

poorbarcode added this to the 3.2.0 milestone Sep 14, 2023

coderzc approved these changes Sep 14, 2023

View reviewed changes

codelipenghui previously approved these changes Sep 15, 2023

View reviewed changes

codelipenghui reviewed Sep 15, 2023

View reviewed changes

poorbarcode requested a review from codelipenghui September 15, 2023 17:01

Technoboy- reviewed Sep 18, 2023

View reviewed changes

...r/src/main/java/org/apache/pulsar/broker/service/AbstractDispatcherSingleActiveConsumer.java Outdated Show resolved Hide resolved

poorbarcode requested a review from Technoboy- September 18, 2023 09:38

poorbarcode force-pushed the fix/exclusive_consumer_instead branch from caf5428 to 653d031 Compare October 11, 2023 03:45

BewareMyPower reviewed Oct 16, 2023

View reviewed changes

RobertIndie reviewed Oct 17, 2023

View reviewed changes

pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java Show resolved Hide resolved

pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java Outdated Show resolved Hide resolved

poorbarcode added 4 commits October 24, 2023 01:18

[fix] [broker] Make the new exclusive consumer instead the inactive o…

9be4fe3

…ne quickly

-

7eb9374

Make the second consumer could success register at the first time of …

2d95026

…subscribe

address comment

34ab245

poorbarcode force-pushed the fix/exclusive_consumer_instead branch from 653d031 to 34ab245 Compare October 23, 2023 17:18

poorbarcode requested review from RobertIndie, coderzc and BewareMyPower October 23, 2023 17:29

Technoboy- approved these changes Oct 24, 2023

View reviewed changes

liangyepianzhou reviewed Oct 24, 2023

View reviewed changes

...r/src/main/java/org/apache/pulsar/broker/service/AbstractDispatcherSingleActiveConsumer.java Outdated Show resolved Hide resolved

poorbarcode removed the release/3.0.2 label Oct 24, 2023

poorbarcode requested a review from liangyepianzhou October 26, 2023 10:15

liangyepianzhou approved these changes Oct 26, 2023

View reviewed changes

eolivelli reviewed Oct 26, 2023

View reviewed changes

...r/src/main/java/org/apache/pulsar/broker/service/AbstractDispatcherSingleActiveConsumer.java Outdated Show resolved Hide resolved

poorbarcode added 2 commits October 26, 2023 18:33

add comment for a test

a277f6e

address comment

f031d99

RobertIndie approved these changes Oct 26, 2023

View reviewed changes

BewareMyPower reviewed Oct 26, 2023

View reviewed changes

pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java Outdated Show resolved Hide resolved

rename a test

863e34d

poorbarcode requested review from BewareMyPower and eolivelli October 27, 2023 06:57

BewareMyPower approved these changes Oct 27, 2023

View reviewed changes

poorbarcode added 3 commits October 29, 2023 07:56

fix test

6c58430

fix test

bed0f7f

fix test

0d4b203

poorbarcode merged commit 29db8f8 into apache:master Oct 30, 2023
47 checks passed

poorbarcode added the cherry-picked/branch-3.0 label Oct 31, 2023

poorbarcode added the cherry-picked/branch-2.11 label Oct 31, 2023

poorbarcode added the cherry-picked/branch-2.10 label Oct 31, 2023

shibd added release/2.11.3 and removed release/2.11.4 labels Dec 20, 2023

Technoboy- mentioned this pull request Jan 10, 2024

[Bug] [worker] ProducerFencedException：Topic has an existing exclusive producer #21809

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] [broker] Make the new exclusive consumer instead the inactive one faster #21183

[fix] [broker] Make the new exclusive consumer instead the inactive one faster #21183

poorbarcode commented Sep 14, 2023 •

edited

Loading

codelipenghui Sep 15, 2023

poorbarcode Sep 15, 2023 •

edited

Loading

poorbarcode Oct 23, 2023 •

edited

Loading

poorbarcode commented Oct 11, 2023

BewareMyPower left a comment

BewareMyPower Oct 16, 2023

poorbarcode Oct 23, 2023

poorbarcode Oct 23, 2023

poorbarcode Oct 26, 2023

[fix] [broker] Make the new exclusive consumer instead the inactive one faster #21183

[fix] [broker] Make the new exclusive consumer instead the inactive one faster #21183

Conversation

poorbarcode commented Sep 14, 2023 • edited Loading

Motivation

Modifications

Documentation

Matching PR in forked repository

codelipenghui Sep 15, 2023

Choose a reason for hiding this comment

poorbarcode Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

poorbarcode Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

poorbarcode commented Oct 11, 2023

BewareMyPower left a comment

Choose a reason for hiding this comment

BewareMyPower Oct 16, 2023

Choose a reason for hiding this comment

poorbarcode Oct 23, 2023

Choose a reason for hiding this comment

poorbarcode Oct 23, 2023

Choose a reason for hiding this comment

poorbarcode Oct 26, 2023

Choose a reason for hiding this comment

poorbarcode commented Sep 14, 2023 •

edited

Loading

poorbarcode Sep 15, 2023 •

edited

Loading

poorbarcode Oct 23, 2023 •

edited

Loading