[fix][broker] Return if AbstractDispatcherSingleActiveConsumer closed #19934

AnonHxy · 2023-03-27T09:03:07Z

Motivation

We should disconect the consumer and return if AbstractDispatcherSingleActiveConsumer is closed

Modifications

Return after consumer disconnect

Verifying this change

Make sure that the change passes the CI checks.

Documentation

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

Matching PR in forked repository

PR in forked repository: AnonHxy#33

poorbarcode

I saw that NonPersistentStickyKeyDispatcherMultipleConsumers and PersistentStickyKeyDispatcherMultipleConsumers also have the same problem, so we can fix them together?

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/nonpersistent/NonPersistentStickyKeyDispatcherMultipleConsumers.java

Lines 86 to 95 in 55523ac

    
           public synchronized void addConsumer(Consumer consumer) throws BrokerServiceException { 
        
               super.addConsumer(consumer); 
        
               try { 
        
                   selector.addConsumer(consumer); 
        
               } catch (BrokerServiceException e) { 
        
                   consumerSet.removeAll(consumer); 
        
                   consumerList.remove(consumer); 
        
                   throw e; 
        
               } 
        
           }

Like the line 89 above, it should not be executed.

And could you write a test for these cases?

AnonHxy · 2023-03-30T12:18:38Z

PersistentStickyKeyDispatcherMultipleConsumers

Good catch @poorbarcode , PTAL again

codecov-commenter · 2023-03-31T03:01:14Z

Codecov Report

Merging #19934 (189b1e3) into master (1e44ba1) will increase coverage by 48.44%.
The diff coverage is 85.55%.

@@              Coverage Diff              @@
##             master   #19934       +/-   ##
=============================================
+ Coverage     24.42%   72.87%   +48.44%     
- Complexity      291    31615    +31324     
=============================================
  Files          1603     1861      +258     
  Lines        124343   137366    +13023     
  Branches      13571    15117     +1546     
=============================================
+ Hits          30369   100101    +69732     
+ Misses        89490    29323    -60167     
- Partials       4484     7942     +3458

Flag	Coverage Δ
inttests	`24.34% <18.88%> (-0.08%)`	⬇️
systests	`25.07% <18.88%> (?)`
unittests	`72.15% <85.55%> (?)`

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

Impacted Files	Coverage Δ
...extensions/reporter/TopBundleLoadDataReporter.java	`88.88% <ø> (+88.88%)`	⬆️
...tent/NonPersistentDispatcherMultipleConsumers.java	`71.60% <ø> (+71.60%)`	⬆️
...dbalance/extensions/scheduler/TransferShedder.java	`81.43% <50.00%> (+81.43%)`	⬆️
...ar/metadata/coordination/impl/LockManagerImpl.java	`89.47% <71.42%> (+31.57%)`	⬆️
...metadata/coordination/impl/LeaderElectionImpl.java	`77.56% <75.00%> (+24.66%)`	⬆️
...java/org/apache/pulsar/common/util/FutureUtil.java	`80.55% <76.92%> (+50.02%)`	⬆️
...ker/loadbalance/extensions/models/TopKBundles.java	`90.78% <87.17%> (+90.78%)`	⬆️
...org/apache/pulsar/broker/ServiceConfiguration.java	`99.37% <100.00%> (+1.46%)`	⬆️
...nsions/policies/AntiAffinityGroupPolicyHelper.java	`90.62% <100.00%> (+90.62%)`	⬆️
...ervice/AbstractDispatcherSingleActiveConsumer.java	`92.37% <100.00%> (+24.85%)`	⬆️
... and 2 more

... and 1568 files with indirect coverage changes

poorbarcode · 2023-03-31T05:45:26Z

...e/pulsar/broker/service/nonpersistent/NonPersistentStickyKeyDispatcherMultipleConsumers.java

+            log.warn("[{}] Dispatcher is already closed. Closing consumer {}", name, consumer);
+            consumer.disconnect();
+            return;
+        }


super.addConsumer(consumer) already did this check, right? If yes, we should not repeatedly check it.

poorbarcode · 2023-03-31T05:45:32Z

.../apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java

+            log.warn("[{}] Dispatcher is already closed. Closing consumer {}", name, consumer);
+            consumer.disconnect();
+            return;
+        }
        super.addConsumer(consumer);


super.addConsumer(consumer) already did this check, right? If yes, we should not repeatedly check it.

However the super.addConsumer return value is void. The method in subClass will not fast return if super.addConsumer return. @poorbarcode

Is there any other comments :) @poorbarcode

@Technoboy- What do you think about this?

yes, super.addConsumer(consumer) can't fast return.

poorbarcode · 2023-03-31T05:47:37Z

...lsar/broker/service/nonpersistent/NonPersistentStickyKeyDispatcherMultipleConsumersTest.java

+        Consumer consumer = mock(Consumer.class);
+        nonpersistentDispatcher.addConsumer(consumer);
+        verify(consumer, times(1)).disconnect();
+        assertEquals(0, nonpersistentDispatcher.getConsumers().size());


Should we add a verify: this consumer does not exist in the selector?

OK. I will update it later

I find that it's not easy to access the selector, and verify assertEquals(0, nonpersistentDispatcher.getConsumers().size()); is enough. So maybe we needn't change it @poorbarcode

poorbarcode · 2023-03-31T05:47:42Z

...che/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumersTest.java

+        Consumer consumer = mock(Consumer.class);
+        persistentDispatcher.addConsumer(consumer);
+        verify(consumer, times(1)).disconnect();
+        assertEquals(0, persistentDispatcher.getConsumers().size());


Should we add a verify: this consumer does not exist in the selector?

…#19934)

michaeljmarshall · 2023-06-27T21:56:04Z

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

…#19934) (cherry picked from commit 42a6969)

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

AnonHxy added the type/bug The PR fixed a bug or issue reported a bug label Mar 27, 2023

AnonHxy mentioned this pull request Mar 27, 2023

Return if AbstractDispatcherSingleActiveConsumer closed AnonHxy/pulsar#33

Closed

AnonHxy self-assigned this Mar 27, 2023

AnonHxy added cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 cherry-picked/branch-2.11 labels Mar 27, 2023

AnonHxy added this to the 3.0.0 milestone Mar 28, 2023

Technoboy- added release/2.8.5 release/2.10.5 release/2.9.6 release/2.11.2 ready-to-test and removed cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 cherry-picked/branch-2.11 labels Mar 30, 2023

Technoboy- approved these changes Mar 30, 2023

View reviewed changes

Technoboy- closed this Mar 30, 2023

Technoboy- reopened this Mar 30, 2023

poorbarcode approved these changes Mar 30, 2023

View reviewed changes

poorbarcode reviewed Mar 30, 2023

View reviewed changes

poorbarcode reviewed Mar 31, 2023

View reviewed changes

AnonHxy added 3 commits April 6, 2023 13:56

Return if AbstractDispatcherSingleActiveConsumer closed

45357dc

Address comment

7d95050

address comment

37ea499

AnonHxy force-pushed the fix_return branch from 893e663 to 37ea499 Compare April 6, 2023 05:56

Technoboy- approved these changes Apr 6, 2023

View reviewed changes

poorbarcode approved these changes Apr 6, 2023

View reviewed changes

Technoboy- merged commit 42a6969 into apache:master Apr 6, 2023

Technoboy- pushed a commit that referenced this pull request Apr 6, 2023

[fix][broker] Return if AbstractDispatcherSingleActiveConsumer closed (…

644c6e4

…#19934)

Technoboy- added cherry-picked/branch-2.11 release/2.11.1 and removed release/2.11.2 labels Apr 6, 2023

Technoboy- added a commit to Technoboy-/pulsar that referenced this pull request Apr 7, 2023

Fix cherry-pick apache#19934 cause compile issue.

c9ae433

michaeljmarshall removed the release/2.9.6 label Jun 27, 2023

liangyepianzhou pushed a commit that referenced this pull request Jul 7, 2023

[fix][broker] Return if AbstractDispatcherSingleActiveConsumer closed (…

c1b3024

…#19934) (cherry picked from commit 42a6969)

liangyepianzhou added the cherry-picked/branch-2.10 label Jul 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][broker] Return if AbstractDispatcherSingleActiveConsumer closed #19934

[fix][broker] Return if AbstractDispatcherSingleActiveConsumer closed #19934

AnonHxy commented Mar 27, 2023 •

edited

Loading

poorbarcode left a comment •

edited

Loading

AnonHxy commented Mar 30, 2023 •

edited

Loading

codecov-commenter commented Mar 31, 2023

poorbarcode Mar 31, 2023

poorbarcode Mar 31, 2023

AnonHxy Mar 31, 2023

AnonHxy Apr 4, 2023

poorbarcode Apr 4, 2023

Technoboy- Apr 5, 2023

poorbarcode Mar 31, 2023

AnonHxy Mar 31, 2023

AnonHxy Apr 4, 2023

AnonHxy Apr 6, 2023

poorbarcode Mar 31, 2023

AnonHxy Apr 6, 2023

michaeljmarshall commented Jun 27, 2023

	public synchronized void addConsumer(Consumer consumer) throws BrokerServiceException {
	super.addConsumer(consumer);
	try {
	selector.addConsumer(consumer);
	} catch (BrokerServiceException e) {
	consumerSet.removeAll(consumer);
	consumerList.remove(consumer);
	throw e;
	}
	}

[fix][broker] Return if AbstractDispatcherSingleActiveConsumer closed #19934

[fix][broker] Return if AbstractDispatcherSingleActiveConsumer closed #19934

Conversation

AnonHxy commented Mar 27, 2023 • edited Loading

Motivation

Modifications

Verifying this change

Documentation

Matching PR in forked repository

poorbarcode left a comment • edited Loading

Choose a reason for hiding this comment

AnonHxy commented Mar 30, 2023 • edited Loading

codecov-commenter commented Mar 31, 2023

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaeljmarshall commented Jun 27, 2023

AnonHxy commented Mar 27, 2023 •

edited

Loading

poorbarcode left a comment •

edited

Loading

AnonHxy commented Mar 30, 2023 •

edited

Loading