You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by GitBox <gi...@apache.org> on 2022/08/08 07:12:42 UTC

[GitHub] [rocketmq] fujian-zfj opened a new pull request, #4797: Develop fix

fujian-zfj opened a new pull request, #4797:
URL: https://github.com/apache/rocketmq/pull/4797

   BUG REPORT
   
   ![image](https://user-images.githubusercontent.com/10379042/183360492-851f29c4-5bf9-4776-b8ff-cc4c45abb65c.png)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] github-actions[bot] commented on pull request #4797: [ISSUE #4771]In HA mode, if you do not truncate consumerOffsetMap, it may cause a bug

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4797:
URL: https://github.com/apache/rocketmq/pull/4797#issuecomment-1703968662

   This PR was closed because it has been inactive for 3 days since being marked as stale.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] RongtongJin commented on a diff in pull request #4797: [ISSUE #4771]In HA mode, if you do not truncate consumerOffsetMap, it may cause a bug

Posted by GitBox <gi...@apache.org>.
RongtongJin commented on code in PR #4797:
URL: https://github.com/apache/rocketmq/pull/4797#discussion_r942048309


##########
store/src/main/java/org/apache/rocketmq/store/TruncateFilesHook.java:
##########
@@ -0,0 +1,14 @@
+package org.apache.rocketmq.store;

Review Comment:
   Missing ASF header



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] lizhanhui commented on pull request #4797: [ISSUE #4771]In HA mode, if you do not truncate consumerOffsetMap, it may cause a bug

Posted by GitBox <gi...@apache.org>.
lizhanhui commented on PR #4797:
URL: https://github.com/apache/rocketmq/pull/4797#issuecomment-1211525248

   As I pointed out [here](https://github.com/apache/rocketmq/issues/4771#issuecomment-1211515886), this fix is kind of walkaround. Should we invest more efforts on the design flaw or correct the design issue itself?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] RongtongJin commented on a diff in pull request #4797: [ISSUE #4771]In HA mode, if you do not truncate consumerOffsetMap, it may cause a bug

Posted by GitBox <gi...@apache.org>.
RongtongJin commented on code in PR #4797:
URL: https://github.com/apache/rocketmq/pull/4797#discussion_r939931107


##########
store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java:
##########
@@ -168,6 +168,8 @@ public class DefaultMessageStore implements MessageStore {
     private final ConcurrentMap<Integer /* level */, Long/* delay timeMillis */> delayLevelTable =
         new ConcurrentHashMap<Integer, Long>(32);
 
+    private ConcurrentMap<String/* topic@group */, ConcurrentMap<Integer, Long>> offsetTable;

Review Comment:
   There is no need to add a consumer offset map here. Perhaps we can modify the consummeOffsetManager of the broker module through the register hook.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] RongtongJin commented on a diff in pull request #4797: [ISSUE #4771]In HA mode, if you do not truncate consumerOffsetMap, it may cause a bug

Posted by GitBox <gi...@apache.org>.
RongtongJin commented on code in PR #4797:
URL: https://github.com/apache/rocketmq/pull/4797#discussion_r939931107


##########
store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java:
##########
@@ -168,6 +168,8 @@ public class DefaultMessageStore implements MessageStore {
     private final ConcurrentMap<Integer /* level */, Long/* delay timeMillis */> delayLevelTable =
         new ConcurrentHashMap<Integer, Long>(32);
 
+    private ConcurrentMap<String/* topic@group */, ConcurrentMap<Integer, Long>> offsetTable;

Review Comment:
   There is no need to add a consumer offset map here. Perhaps we can modify the consummeQueueStore of the broker module through the register hook.



##########
store/src/main/java/org/apache/rocketmq/store/timer/TimerMessageStore.java:
##########
@@ -81,7 +81,7 @@ public class TimerMessageStore {
     public static final int DAY_SECS = 24 * 3600;
     // The total days in the timer wheel when precision is 1000ms.
     // If the broker shutdown last more than the configured days, will cause message loss
-    public static final int TIMER_WHELL_TTL_DAY = 7;
+    public static final int TIMER_WHEEL_TTL_DAY = 7;

Review Comment:
   Modifications not related to this PR can be split



##########
store/src/main/java/org/apache/rocketmq/store/queue/ConsumeQueueStore.java:
##########
@@ -341,6 +343,50 @@ private void putConsumeQueue(final String topic, final int queueId, final Consum
         }
     }
 
+    public void truncateTimerConsumerOffset(TimerMessageStore timerMessageStore) {
+        if (this.consumeQueueTable.get(TimerMessageStore.TIMER_TOPIC) == null
+            || this.consumeQueueTable.get(TimerMessageStore.TIMER_TOPIC).get(0) == null) {
+            return;
+        }
+        ConsumeQueueInterface consumerQueue = this.consumeQueueTable.get(TimerMessageStore.TIMER_TOPIC).get(0);
+        if (timerMessageStore.getQueueOffset() > consumerQueue.getMaxOffsetInQueue()) {
+            timerMessageStore.setQueueOffset(consumerQueue.getMaxOffsetInQueue());
+        }
+    }

Review Comment:
   It would be better to handle timer consumer offset separately in consume queue store.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] dongeforever commented on pull request #4797: [ISSUE #4771]In HA mode, if you do not truncate consumerOffsetMap, it may cause a bug

Posted by GitBox <gi...@apache.org>.
dongeforever commented on PR #4797:
URL: https://github.com/apache/rocketmq/pull/4797#issuecomment-1207912284

   @RongtongJin The truncation may also need to be done when the broker role changes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] RongtongJin commented on a diff in pull request #4797: [ISSUE #4771]In HA mode, if you do not truncate consumerOffsetMap, it may cause a bug

Posted by GitBox <gi...@apache.org>.
RongtongJin commented on code in PR #4797:
URL: https://github.com/apache/rocketmq/pull/4797#discussion_r939933737


##########
store/src/main/java/org/apache/rocketmq/store/queue/ConsumeQueueStore.java:
##########
@@ -341,6 +343,50 @@ private void putConsumeQueue(final String topic, final int queueId, final Consum
         }
     }
 
+    public void truncateTimerConsumerOffset(TimerMessageStore timerMessageStore) {
+        if (this.consumeQueueTable.get(TimerMessageStore.TIMER_TOPIC) == null
+            || this.consumeQueueTable.get(TimerMessageStore.TIMER_TOPIC).get(0) == null) {
+            return;
+        }
+        ConsumeQueueInterface consumerQueue = this.consumeQueueTable.get(TimerMessageStore.TIMER_TOPIC).get(0);
+        if (timerMessageStore.getQueueOffset() > consumerQueue.getMaxOffsetInQueue()) {
+            timerMessageStore.setQueueOffset(consumerQueue.getMaxOffsetInQueue());
+        }
+    }

Review Comment:
   It would be better to handle timer consumer offset separately in timerMessageStore.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] github-actions[bot] commented on pull request #4797: [ISSUE #4771]In HA mode, if you do not truncate consumerOffsetMap, it may cause a bug

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4797:
URL: https://github.com/apache/rocketmq/pull/4797#issuecomment-1698301908

   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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] ShannonDing commented on a diff in pull request #4797: [ISSUE #4771]In HA mode, if you do not truncate consumerOffsetMap, it may cause a bug

Posted by GitBox <gi...@apache.org>.
ShannonDing commented on code in PR #4797:
URL: https://github.com/apache/rocketmq/pull/4797#discussion_r939947871


##########
store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java:
##########
@@ -646,6 +648,11 @@ public void truncateDirtyFiles(long offsetToTruncate) {
 
         recoverTopicQueueTable();
 
+        // truncate consumer offset
+        truncateConsumerOffsetTable(this.offsetTable);

Review Comment:
   truncateConsumerOffsetTable() is OK, the parameter is not required.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] hzh0425 commented on pull request #4797: [ISSUE #4771]In HA mode, if you do not truncate consumerOffsetMap, it may cause a bug

Posted by GitBox <gi...@apache.org>.
hzh0425 commented on PR #4797:
URL: https://github.com/apache/rocketmq/pull/4797#issuecomment-1207829941

   Good point!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] RongtongJin commented on pull request #4797: [ISSUE #4771]In HA mode, if you do not truncate consumerOffsetMap, it may cause a bug

Posted by GitBox <gi...@apache.org>.
RongtongJin commented on PR #4797:
URL: https://github.com/apache/rocketmq/pull/4797#issuecomment-1208849452

   > @RongtongJin The truncation may also need to be done when the broker role changes.
   
   Yes, when the broker role changes, commit log and consume queue have been truncated, but consumer offset has not been corrected. This PR will correct consumer offset.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] codecov-commenter commented on pull request #4797: [ISSUE #4771]In HA mode, if you do not truncate consumerOffsetMap, it may cause a bug

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #4797:
URL: https://github.com/apache/rocketmq/pull/4797#issuecomment-1229425600

   # [Codecov](https://codecov.io/gh/apache/rocketmq/pull/4797?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#4797](https://codecov.io/gh/apache/rocketmq/pull/4797?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (48968f3) into [develop](https://codecov.io/gh/apache/rocketmq/commit/64c18fb0dbb8d7e0e4af9edf49d04ab8dd64bccd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (64c18fb) will **decrease** coverage by `0.00%`.
   > The diff coverage is `2.43%`.
   
   ```diff
   @@              Coverage Diff              @@
   ##             develop    #4797      +/-   ##
   =============================================
   - Coverage      43.29%   43.29%   -0.01%     
   - Complexity      7688     7703      +15     
   =============================================
     Files            991      994       +3     
     Lines          68781    68979     +198     
     Branches        9111     9144      +33     
   =============================================
   + Hits           29782    29866      +84     
   - Misses         35253    35365     +112     
   - Partials        3746     3748       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/rocketmq/pull/4797?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [.../rocketmq/broker/offset/ConsumerOffsetManager.java](https://codecov.io/gh/apache/rocketmq/pull/4797/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9icm9rZXIvb2Zmc2V0L0NvbnN1bWVyT2Zmc2V0TWFuYWdlci5qYXZh) | `27.89% <0.00%> (-4.23%)` | :arrow_down: |
   | [...n/java/org/apache/rocketmq/store/MessageStore.java](https://codecov.io/gh/apache/rocketmq/pull/4797/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL01lc3NhZ2VTdG9yZS5qYXZh) | `0.00% <ø> (ø)` | |
   | [...apache/rocketmq/store/timer/TimerMessageStore.java](https://codecov.io/gh/apache/rocketmq/pull/4797/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL3RpbWVyL1RpbWVyTWVzc2FnZVN0b3JlLmphdmE=) | `53.70% <0.00%> (-0.59%)` | :arrow_down: |
   | [...org/apache/rocketmq/store/DefaultMessageStore.java](https://codecov.io/gh/apache/rocketmq/pull/4797/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL0RlZmF1bHRNZXNzYWdlU3RvcmUuamF2YQ==) | `55.23% <14.28%> (-0.23%)` | :arrow_down: |
   | [...or/validator/DefaultTopicMessageTypeValidator.java](https://codecov.io/gh/apache/rocketmq/pull/4797/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHJveHkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3Byb3h5L3Byb2Nlc3Nvci92YWxpZGF0b3IvRGVmYXVsdFRvcGljTWVzc2FnZVR5cGVWYWxpZGF0b3IuamF2YQ==) | `40.00% <0.00%> (-10.00%)` | :arrow_down: |
   | [...e/rocketmq/controller/impl/manager/BrokerInfo.java](https://codecov.io/gh/apache/rocketmq/pull/4797/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvY29udHJvbGxlci9pbXBsL21hbmFnZXIvQnJva2VySW5mby5qYXZh) | `78.94% <0.00%> (-9.29%)` | :arrow_down: |
   | [...ocketmq/controller/impl/manager/SyncStateInfo.java](https://codecov.io/gh/apache/rocketmq/pull/4797/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvY29udHJvbGxlci9pbXBsL21hbmFnZXIvU3luY1N0YXRlSW5mby5qYXZh) | `83.33% <0.00%> (-7.58%)` | :arrow_down: |
   | [...he/rocketmq/controller/impl/DLedgerController.java](https://codecov.io/gh/apache/rocketmq/pull/4797/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvY29udHJvbGxlci9pbXBsL0RMZWRnZXJDb250cm9sbGVyLmphdmE=) | `69.51% <0.00%> (-3.46%)` | :arrow_down: |
   | [...ketmq/client/impl/consumer/PullMessageService.java](https://codecov.io/gh/apache/rocketmq/pull/4797/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9jb25zdW1lci9QdWxsTWVzc2FnZVNlcnZpY2UuamF2YQ==) | `49.23% <0.00%> (-3.08%)` | :arrow_down: |
   | [...ache/rocketmq/controller/impl/event/EventType.java](https://codecov.io/gh/apache/rocketmq/pull/4797/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvY29udHJvbGxlci9pbXBsL2V2ZW50L0V2ZW50VHlwZS5qYXZh) | `73.68% <0.00%> (-2.79%)` | :arrow_down: |
   | ... and [36 more](https://codecov.io/gh/apache/rocketmq/pull/4797/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [rocketmq] github-actions[bot] closed pull request #4797: [ISSUE #4771]In HA mode, if you do not truncate consumerOffsetMap, it may cause a bug

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #4797: [ISSUE #4771]In HA mode, if you do not truncate consumerOffsetMap, it may cause a bug
URL: https://github.com/apache/rocketmq/pull/4797


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org