-
Notifications
You must be signed in to change notification settings - Fork 11.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 #6681] fix: fix pop retry message notification #6682
Conversation
Pop retry message has already been notified in PopReviveService#reviveRetry
|
In But in the case which rocketmq/broker/src/main/java/org/apache/rocketmq/broker/processor/QueryAssignmentProcessor.java Lines 236 to 270 in 6f6032e
In this case, pollingMap only contains polling requests of specific queues, and notifying -1 queueID will not wake up the polling requests. The polling requests will finally be waked up until they reach the expiration time. |
Got it |
.putIfAbsent(requestHeader.getConsumerGroup(), Byte.MIN_VALUE); | ||
final String popRetryTopic | ||
= KeyBuilder.buildPopRetryTopic(requestHeader.getTopic(), requestHeader.getConsumerGroup()); | ||
topicCidMap.computeIfAbsent(popRetryTopic, k -> new ConcurrentHashMap<>()) |
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.
Why we need put popRetryTopic into topicCidMap? In notifyRetryMessageArriving, the topic has been changed to normal topic.
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.
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.
But I didn't notice notifyMessageArriving
is called in PopReviveService#reviveRetry
.
It looks like it would be more appropriate to call notifyRetryMessageArriving
in PopReviveService#reviveRetry
.
054bc37
to
b823d35
Compare
Codecov Report
@@ Coverage Diff @@
## develop #6682 +/- ##
=============================================
- Coverage 42.65% 42.62% -0.03%
+ Complexity 9093 9086 -7
=============================================
Files 1121 1121
Lines 79775 79795 +20
Branches 10409 10415 +6
=============================================
- Hits 34027 34015 -12
- Misses 41462 41500 +38
+ Partials 4286 4280 -6
... and 31 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@xdkxlk PTAL |
b823d35
to
0366273
Compare
This PR is stale because it has been open for 365 days with no activity. It will be closed in 3 days if no further activity occurs. If you wish not to mark it as stale, please leave a comment in this PR. |
This PR was closed because it has been inactive for 3 days since being marked as stale. |
Which Issue(s) This PR Fixes
Fixes #6681
Brief Description
After
ReputMessageService
dispatch a message from a pop retry topic, notify all consumers who subscribe to any queue of this topic.How Did You Test This Change?
TopicTest
.PopConsumer.java
to subscribeTopicTest
, set popBatchSize to 1, and when receiving a message, print it and returnRECONSUME_LATER
.TopicTest
.The consumer consume retry message immediately