Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][client] Messages with inconsistent consumer epochs are not filtered when using batch receive and trigger timeout #18478

Merged
merged 6 commits into from
Nov 19, 2022

Conversation

Technoboy-
Copy link
Contributor

@Technoboy- Technoboy- commented Nov 15, 2022

Motivation

The original patch is #17318. Revert by #18475

Messages with inconsistent consumer epochs are not filtered when using batch receive and trigger timeout.

Modifications

  • Add consumer epoch on notifyPendingBatchReceivedCallBack.
  • Reuse notifyPendingBatchReceivedCallBack logic.

Verifying this change

  • testBatchReceiveRedeliveryAddEpoch unit test covers the scene.

Documentation

  • doc-not-needed

@Technoboy- Technoboy- self-assigned this Nov 15, 2022
@Technoboy- Technoboy- added this to the 2.12.0 milestone Nov 15, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 15, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2022

Codecov Report

Merging #18478 (61cff54) into master (8b404d4) will increase coverage by 0.06%.
The diff coverage is 6.12%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18478      +/-   ##
============================================
+ Coverage     47.15%   47.21%   +0.06%     
- Complexity    10432    10440       +8     
============================================
  Files           697      698       +1     
  Lines         68023    67986      -37     
  Branches       7284     7271      -13     
============================================
+ Hits          32074    32101      +27     
+ Misses        32367    32306      -61     
+ Partials       3582     3579       -3     
Flag Coverage Δ
unittests 47.21% <6.12%> (+0.06%) ⬆️

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

Impacted Files Coverage Δ
...he/pulsar/client/impl/MultiTopicsConsumerImpl.java 22.98% <0.00%> (+0.71%) ⬆️
...n/java/org/apache/pulsar/client/util/NoOpLock.java 0.00% <0.00%> (ø)
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 15.12% <7.69%> (+0.28%) ⬆️
...va/org/apache/pulsar/client/impl/ConsumerBase.java 21.93% <16.66%> (-0.02%) ⬇️
...oker/loadbalance/LoadResourceQuotaUpdaterTask.java 44.44% <0.00%> (-33.34%) ⬇️
.../apache/pulsar/broker/loadbalance/LoadManager.java 61.11% <0.00%> (-16.67%) ⬇️
...tent/NonPersistentDispatcherMultipleConsumers.java 40.74% <0.00%> (-12.35%) ⬇️
...apache/pulsar/broker/service/TopicListService.java 42.62% <0.00%> (-12.30%) ⬇️
...pulsar/broker/service/PulsarCommandSenderImpl.java 72.30% <0.00%> (-6.16%) ⬇️
...ction/buffer/impl/TransactionBufferClientImpl.java 76.74% <0.00%> (-4.66%) ⬇️
... and 43 more

clearIncomingMessages();
unAckedMessageTracker.clear();
int currentSize;
synchronized (incomingQueueLock) {
Copy link
Member

Choose a reason for hiding this comment

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

It seems that locks still need to be used. if use share sub, may we don't need this lock.

Original comment: #17318 (comment)

Related modifications: f2d6b47

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

@Technoboy- Technoboy- closed this Nov 16, 2022
@Technoboy- Technoboy- reopened this Nov 16, 2022
@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

@codelipenghui
Copy link
Contributor

@Technoboy- Please help resolve the conflicts

@Technoboy-
Copy link
Contributor Author

@Technoboy- Please help resolve the conflicts

Done

@Technoboy- Technoboy- merged commit 7cc8660 into apache:master Nov 19, 2022
Comment on lines +851 to +853
incomingQueueLock.lock();
try {
if (canEnqueueMessage(message) && incomingMessages.offer(message)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

add lock but don't check the epoch, it also can't filter the message that the epoch is smaller than the current epoch, right? @Technoboy- @codelipenghui

Copy link
Member

Choose a reason for hiding this comment

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

+1, here need add judge isValidConsumerEpoch(message)

lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Dec 9, 2022
@Technoboy- Technoboy- deleted the fix-17318 branch November 11, 2023 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants