Skip to content

Conversation

@poorbarcode
Copy link
Contributor

Fixes #16795

Motivation

In class PersistentDispatcherMultipleConsumers, there is a variable sendInProgress that is set to true before the sendMessagesToConsumers method is executed and to false after the sendMessagesToConsumers method is executed, thus making the readMoreEnties inaccessible during the sendMessagesToConsumers method's execution. see: #16812

In subclass PersistentStreamingDispatcherMultipleConsumers, when reading batch entries, sendMessagesToConsumer is called each entry is read, which causes the variable sendInProgress to be set to true and false multiple times. This results in the method readMoreEnties can be accessed when the variable sendInProgress is set to false when a batch of entries is reading. This makes repeat delivery of the message, the error process is as follows:

step consumer-1 consumer-2
1 try load messages
2 peek message from the queue messagesToRedeliver try load messages
3 repay messages peek message from the queue messagesToRedeliver
4 send message to consumer-1 repay messages
5 send message to consumer-2

Modifications

Set sendInProgress to false only after the last entry in a batch of messages has been delivered.

Documentation

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

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 1, 2022
PulsarClient newPulsarClient4 = newPulsarClient(lookupUrl.toString(), 0);// Creates new client connection
Consumer<byte[]> c5 = newPulsarClient4.newConsumer()
.topic("persistent://my-property/my-ns/my-topic2").subscriptionName("my-subscriber-name")
.consumerName("c5")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need to remove the consumer name here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already fixed

@codelipenghui codelipenghui added this to the 2.12.0 milestone Nov 2, 2022
@codelipenghui codelipenghui added type/bug The PR fixed a bug or issue reported a bug area/broker type/flaky-tests labels Nov 2, 2022
@poorbarcode poorbarcode force-pushed the flaky/testSharedSamePriorityConsumer branch from 293af82 to 11f101a Compare November 2, 2022 03:23
@poorbarcode poorbarcode closed this Nov 2, 2022
@poorbarcode poorbarcode reopened this Nov 2, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2022

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 41.47%. Comparing base (0866c3a) to head (11f101a).
Report is 2237 commits behind head on master.

Files with missing lines Patch % Lines
...ersistentStreamingDispatcherMultipleConsumers.java 0.00% 6 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18289      +/-   ##
============================================
+ Coverage     38.97%   41.47%   +2.50%     
- Complexity     8311     8864     +553     
============================================
  Files           683      687       +4     
  Lines         67325    67384      +59     
  Branches       7217     7218       +1     
============================================
+ Hits          26239    27949    +1710     
+ Misses        38079    36344    -1735     
- Partials       3007     3091      +84     
Flag Coverage Δ
unittests 41.47% <0.00%> (+2.50%) ⬆️

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

Files with missing lines Coverage Δ
...ersistentStreamingDispatcherMultipleConsumers.java 0.00% <0.00%> (ø)

... and 99 files with indirect coverage changes

@nodece nodece merged commit bf407f2 into apache:master Nov 2, 2022
@poorbarcode poorbarcode deleted the flaky/testSharedSamePriorityConsumer branch November 2, 2022 09:40
@Technoboy- Technoboy- changed the title [fix][broker]fix Repeated messages of shared streaming dispatcher [fix][broker]Fix repeated messages of shared streaming dispatcher Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/broker doc-not-needed Your PR changes do not impact docs type/bug The PR fixed a bug or issue reported a bug type/flaky-tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky-test: SimpleProducerConsumerTest.testSharedSamePriorityConsumer

5 participants