-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[Issue 12885]Fix unordered consuming case in Key_Shared subscription. #12890
[Issue 12885]Fix unordered consuming case in Key_Shared subscription. #12890
Conversation
@codelipenghui @lhotari @eolivelli @merlimat Would you please help review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the case, I think the root cause is there is an ongoing read op to read data from the managed ledger, but after a consumer closed, some messages will be added to the replay queue, after the previous read op completed, the active consumer might get messages out of order by key.
I think we can just add a check after got new messages from the managed ledger? If we have messages in the replay queue, we can just add the new messages to the replay queue and then trigger a new read entries operation, then the new operation will try to replay messages from the replay queue, does this can fix the issue?
The root cause of this case is Line 137 in 5523604
So in the following Thread2, when Line 315 in 5523604
|
ea6aee5
to
7884c0a
Compare
@codelipenghui Come up with a more simple way for the case. PTAL. |
/pulsarbot run-failure-checks |
7884c0a
to
d61c17c
Compare
@merlimat Please help review this PR. |
.../apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java
Outdated
Show resolved
Hide resolved
/pulsarbot run-failure-checks |
5021128
to
f7aa203
Compare
f7aa203
to
ca97899
Compare
* up/master: (75 commits) [website][upgrade]feat: website upgrade / docs migration - 2.5.1 Get Started/Concepts and Architecture/Pulsar Schema (apache#13030) Fix environment variable assignment in startup scripts (apache#13025) update 2.8.x (apache#13029) [Doc] add tips for Pulsar tools (apache#13044) Suggest to use tlsPort instead of deprecated TlsEnable (apache#13039) Integration tests for function-worker rebalance and drain operations. (apache#13058) fix(functions): missing runtime set in GoInstanceConfig (apache#13031) [pulsar-admin] Add get-replicated-subscription-status command for topic (apache#12891) [Broker] Consider topics in pulsar/system namespace as system topics (apache#13050) Fix typo: correct sizeUint to sizeUnit (apache#13040) fix-12894 (apache#12896) Don't attempt to delete pending ack store unless transactions are enabled (apache#13041) [Perf] Evaluate the current protocol version once (apache#13045) Fix Issue apache#12885, Unordered consuming case in Key_Shared subscription (apache#12890) [broker]Optimize topicMaxMessageSize with topic local cache. (apache#12830) [PIP-105] Part-2 Support pluggable entry filter in Dispatcher (apache#12970) [website] Modify admin-api-topic.md document (apache#12996) add missed import (apache#13037) [metadata] Add RocksdbMetadataStore (apache#12776) [C] Add pulsar_client_subscribe_multi_topics and pulsar_client_subscribe_pattern (apache#12965) ... # Conflicts: # site2/website-next/docusaurus.config.js # site2/website-next/versioned_sidebars/version-2.6.1-sidebars.json # site2/website-next/versions.json
…iption (apache#12890) (cherry picked from commit 73ef162)
Fixes #12885
Motivation
See #12885
Modifications
Restrict that the consumer remove happens after entries readings.
Verifying this change
This change is already covered by existing tests, such as KeySharedSubscriptionTest.
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
no-need-doc
Bug fix.