[ISSUE #2732] Fix message loss problem when rebalance with LitePullConsumer #2832
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RongtongJin
approved these changes
Apr 23, 2021
|
Is the range of this lock a bit large? If a consumer allocates more queues, will many pull tasks get stuck in updatePullOffset? |
RongtongJin
reviewed
Apr 23, 2021
| @@ -83,10 +83,15 @@ public long getPullOffset(MessageQueue messageQueue) { | |||
| return -1; | |||
| } | |||
|
|
|||
| public void updatePullOffset(MessageQueue messageQueue, long offset) { | |||
| public void updatePullOffset(MessageQueue messageQueue, long offset, ProcessQueue processQueue) { | |||
| MessageQueueState messageQueueState = assignedMessageQueueState.get(messageQueue); | |||
There was a problem hiding this comment.
If return null here, it may report to NPE.
Comment on lines
88
to
95
| synchronized (this.assignedMessageQueueState) { | ||
| if (messageQueueState.getProcessQueue() != processQueue) { | ||
| return; | ||
| } | ||
| if (messageQueueState != null) { | ||
| messageQueueState.setPullOffset(offset); | ||
| } | ||
| } |
There was a problem hiding this comment.
Synchronized here seems to be unnecessary
There was a problem hiding this comment.
it seems putting the synchronized before assignedMessageQueueState.get will be better.
There was a problem hiding this comment.
It seems the synchronized lock can be removed.
I have update this branch.
Codecov Report
@@ Coverage Diff @@
## develop #2832 +/- ##
=============================================
+ Coverage 46.52% 46.59% +0.06%
- Complexity 3426 3427 +1
=============================================
Files 307 307
Lines 29059 29065 +6
Branches 4172 4175 +3
=============================================
+ Hits 13520 13543 +23
+ Misses 13697 13674 -23
- Partials 1842 1848 +6 Continue to review full report at Codecov.
|
vongosling
approved these changes
Apr 26, 2021
RongtongJin
approved these changes
Apr 27, 2021
vongosling
added a commit
that referenced
this pull request
May 19, 2021
* [ISSUE #1233] Fix CVE-2011-1473 * fix Multiple instances in the same application share MQClientInstance * [ISSUE #2748] Fix deleteSubscriptionGroup not remove consumer offset * [ISSUE #2745] Changed the support time of the request/reply feature to 4.6.0. Co-authored-by: von gosling <vongosling@apache.org> * [ISSUE #2729] Replace with Math.min method call * [ISSUE #2801]Fix NamesrvAddr connot set in Producer * [ISSUE 2800] optimize: the spelling of topicSynFlag Co-authored-by: ph3636 <tianxingguang@kanzhun.com> * [ISSUE #2803] Fix the endpoint cannot get instanceId without http (#2804) * fix the endpoint cannot get instanceId without http * fix the endpoint cannot get instanceId without http * add unit test * add unit test * add unit test Co-authored-by: panzhi33 <wb-pz502261@alibaba-inc.com> * fix messageArrivingListener NPE * [ISSUE #2538]Optimize log output when message trace saving fails * [ISSUE #2811] Fix the wrong topic was consumed in the DefaultMessageStoreTest test program * [ISSUE #2821] Overriding the ServiceThread#shutdown in HAClient class * [ISSUE #2805] remove redundant package imports * [ISSUE #2833] Support trace for TranscationProducer (#2834) * [ISSUE #2732] Fix message loss problem when rebalance with LitePullConsumer (#2832) * [ISSUE #2732] Fix message loss problem when rebalance with LitePullConsumer * Fix message loss problem when rebalance with LitePullConsumer, update 2 * [ISSUE #2846]fix -E might not port to other systems * fix some nonconformity after checkstyle * Support OpenTracing(#2861) * [ISSUE #2872] remove log files created by integration test when mvn clean * [ISSUE #2872] move log files created by integration test to target dir * Change log level to debug: "Half offset {} has been committed/rolled back" * Fix unit test stability Bump mockito-core to 3.10.0, remove powermock dependency, suppress useless logging * [ISSUE #2898] Resolve rocketmq-example project failed during checkstyle execution (#2899) Co-authored-by: SSpirits <shadowyspirits@outlook.com> Co-authored-by: panzhi33 <wb-pz502261@alibaba-inc.com> Co-authored-by: panzhi <panzhi33@qq.com> Co-authored-by: ArronHuang <41609451+ArronHuang@users.noreply.github.com> Co-authored-by: von gosling <vongosling@apache.org> Co-authored-by: drgnchan <40224023+drgnchan@users.noreply.github.com> Co-authored-by: zhangjidi2016 <zhangjidi@cmss.chinamobile.com> Co-authored-by: ph3636 <38041490+ph3636@users.noreply.github.com> Co-authored-by: ph3636 <tianxingguang@kanzhun.com> Co-authored-by: BurningCN <1015773611@qq.com> Co-authored-by: francis lee <francislee.cn@outlook.com> Co-authored-by: 灼华 <43363120+BurningCN@users.noreply.github.com> Co-authored-by: yuz10 <845238369@qq.com> Co-authored-by: huangli <areyouok@gmail.com> Co-authored-by: chenrl <raymond2366@outlook.com> Co-authored-by: ayanamist <ayanamist@gmail.com> Co-authored-by: zhangjidi2016 <1017543663@qq.com>
GenerousMan
pushed a commit
to GenerousMan/rocketmq
that referenced
this pull request
Aug 12, 2022
…PullConsumer (apache#2832) * [ISSUE apache#2732] Fix message loss problem when rebalance with LitePullConsumer * Fix message loss problem when rebalance with LitePullConsumer, update 2
pulllock
pushed a commit
to pulllock/rocketmq
that referenced
this pull request
Oct 19, 2023
…PullConsumer (apache#2832) * [ISSUE apache#2732] Fix message loss problem when rebalance with LitePullConsumer * Fix message loss problem when rebalance with LitePullConsumer, update 2
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make sure set the target branch to
developWhat is the purpose of the change
see issue #2732
this commit will help reproduce the bug : areyouok@a54599e
Brief changelog
Verifying this change
Follow this checklist to help us incorporate your contribution quickly and easily. Notice,
it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR.[ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyleto make sure basic checks pass. Runmvn clean install -DskipITsto make sure unit-test pass. Runmvn clean test-compile failsafe:integration-testto make sure integration-test pass.