Skip to content

Conversation

@aloyszhang
Copy link
Contributor

@aloyszhang aloyszhang commented Feb 21, 2023

Motivation

There're some cases of consumers receiving duplicated messages when there are ongoing transactions within a subscription.
#14327 has fixed this problem partially. But there's still a chance for consumers to receive duplicated messages.

  1. For non-batch messages, the dispatcher does not filter messages in the pendingAck state(PendingAckHandleImpl#individualAckPositions)

  2. For batch messages, if the ackSet in pendingAck state is complimentary with the ackSet in the cursor, the broker still dispatches this entry to the consumer. For example, a batch message has 5 messages internal, cursor stats has acked 0,1,2 and pendingAck stats has acked 3,4, this message should not dispatch to the consumer, but actually, it does.

Modifications

  1. filter messages in pendingAck for non-batch messages
  2. filter messages ackSet in pendingAck state is complimentary with the ackSet in the cursor for batch messages

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

  • doc-not-needed

Matching PR in forked repository

PR in forked repository:
aloyszhang#15

@github-actions
Copy link

@aloyszhang Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

Copy link
Contributor

@congbobo184 congbobo184 left a comment

Choose a reason for hiding this comment

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

LGTM! left some comments

@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2023

Codecov Report

Merging #19581 (2c58abf) into master (e0b50c9) will increase coverage by 37.45%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #19581       +/-   ##
=============================================
+ Coverage     24.65%   62.11%   +37.45%     
- Complexity      291    25735    +25444     
=============================================
  Files          1591     1844      +253     
  Lines        123073   135341    +12268     
  Branches      13434    14884     +1450     
=============================================
+ Hits          30348    84069    +53721     
+ Misses        88233    43533    -44700     
- Partials       4492     7739     +3247     
Flag Coverage Δ
inttests 24.60% <0.00%> (-0.06%) ⬇️
systests 25.18% <0.00%> (?)
unittests 59.29% <100.00%> (?)

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

Impacted Files Coverage Δ
...he/bookkeeper/mledger/util/PositionAckSetUtil.java 83.72% <100.00%> (+37.56%) ⬆️
.../pulsar/broker/service/AbstractBaseDispatcher.java 67.70% <100.00%> (+25.00%) ⬆️
...ce/ConsistentHashingStickyKeyConsumerSelector.java 76.92% <0.00%> (-3.85%) ⬇️
...service/nonpersistent/NonPersistentReplicator.java 58.41% <0.00%> (-1.99%) ⬇️
...in/java/org/apache/pulsar/common/api/AuthData.java 71.42% <0.00%> (ø)
.../apache/pulsar/broker/namespace/LookupOptions.java 87.50% <0.00%> (ø)
.../apache/pulsar/common/naming/SystemTopicNames.java 55.55% <0.00%> (ø)
...apache/pulsar/common/util/SafeCollectionUtils.java 0.00% <0.00%> (ø)
...pache/pulsar/common/configuration/BindAddress.java 22.22% <0.00%> (ø)
...r/client/admin/internal/data/AuthPoliciesImpl.java 65.21% <0.00%> (ø)
... and 1370 more

@aloyszhang aloyszhang merged commit e6bc499 into apache:master Feb 23, 2023
@aloyszhang aloyszhang added this to the 3.0.0 milestone Feb 23, 2023
liangyepianzhou pushed a commit that referenced this pull request Feb 25, 2023
…ngAckHandle (#19581)

Co-authored-by: mayozhang <mayozhang@tencent.com>
(cherry picked from commit e6bc499)
@coderzc
Copy link
Member

coderzc commented Mar 2, 2023

@aloyszhang Can you help cherry-pick this PR to branch-2.9?

Technoboy- pushed a commit that referenced this pull request Mar 6, 2023
…ngAckHandle (#19581)

Co-authored-by: mayozhang <mayozhang@tencent.com>
@coderzc
Copy link
Member

coderzc commented Mar 6, 2023

Due to #14327 be moved 2.9.6, so this PR also move to 2.9.6

@michaeljmarshall
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants