You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "mxsm (via GitHub)" <gi...@apache.org> on 2023/05/02 07:49:33 UTC

[GitHub] [rocketmq] mxsm opened a new pull request, #6680: [ISSUE #6679]SubscriptionData may not be updated when the client's system time is rolled back

mxsm opened a new pull request, #6680:
URL: https://github.com/apache/rocketmq/pull/6680

   <!-- Please make sure the target branch is right. In most case, the target branch should be `develop`. -->
   
   ### Which Issue(s) This PR Fixes
   
   <!-- Please ensure that the related issue has already been created, and [link this pull request to that issue using keywords](<https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword>) to ensure automatic closure. -->
   
   Fixes #6679 
   
   ### Brief Description
   
   - Fix SubscriptionData may not be updated when the client's system time is rolled back
   


-- 
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] codecov-commenter commented on pull request #6680: [ISSUE #6679]SubscriptionData may not be updated when the client's system time is rolled back

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #6680:
URL: https://github.com/apache/rocketmq/pull/6680#issuecomment-1537298370

   ## [Codecov](https://app.codecov.io/gh/apache/rocketmq/pull/6680?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 [#6680](https://app.codecov.io/gh/apache/rocketmq/pull/6680?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d1de8b3) into [develop](https://app.codecov.io/gh/apache/rocketmq/commit/7903c70412fc4d20baafd7b75ca93788551fb88f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7903c70) will **increase** coverage by `0.33%`.
   > The diff coverage is `100.00%`.
   
   > :exclamation: Current head d1de8b3 differs from pull request most recent head 101543c. Consider uploading reports for the commit 101543c to get more accurate results
   
   ```diff
   @@              Coverage Diff              @@
   ##             develop    #6680      +/-   ##
   =============================================
   + Coverage      43.04%   43.38%   +0.33%     
   - Complexity      9019     9325     +306     
   =============================================
     Files           1109     1116       +7     
     Lines          78591    81261    +2670     
     Branches       10226    10948     +722     
   =============================================
   + Hits           33831    35255    +1424     
   - Misses         40527    41666    +1139     
   - Partials        4233     4340     +107     
   ```
   
   
   | [Impacted Files](https://app.codecov.io/gh/apache/rocketmq/pull/6680?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...cketmq/client/impl/consumer/RebalancePushImpl.java](https://app.codecov.io/gh/apache/rocketmq/pull/6680?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9jb25zdW1lci9SZWJhbGFuY2VQdXNoSW1wbC5qYXZh) | `49.18% <100.00%> (ø)` | |
   
   ... and [39 files with indirect coverage changes](https://app.codecov.io/gh/apache/rocketmq/pull/6680/indirect-changes?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: commits-unsubscribe@rocketmq.apache.org

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

[GitHub] [rocketmq] ferrirW commented on a diff in pull request #6680: [ISSUE #6679]SubscriptionData may not be updated when the client's system time is rolled back

Posted by "ferrirW (via GitHub)" <gi...@apache.org>.
ferrirW commented on code in PR #6680:
URL: https://github.com/apache/rocketmq/pull/6680#discussion_r1184807789


##########
client/src/main/java/org/apache/rocketmq/client/impl/consumer/RebalancePushImpl.java:
##########
@@ -57,7 +57,8 @@ public void messageQueueChanged(String topic, Set<MessageQueue> mqAll, Set<Messa
          * Fix: inconsistency subscription may lead to consumer miss messages.
          */
         SubscriptionData subscriptionData = this.subscriptionInner.get(topic);
-        long newVersion = System.currentTimeMillis();

Review Comment:
   Maybe it could be nanoseconds



-- 
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] mxsm commented on a diff in pull request #6680: [ISSUE #6679]SubscriptionData may not be updated when the client's system time is rolled back

Posted by "mxsm (via GitHub)" <gi...@apache.org>.
mxsm commented on code in PR #6680:
URL: https://github.com/apache/rocketmq/pull/6680#discussion_r1185168051


##########
client/src/main/java/org/apache/rocketmq/client/impl/consumer/RebalancePushImpl.java:
##########
@@ -57,7 +57,8 @@ public void messageQueueChanged(String topic, Set<MessageQueue> mqAll, Set<Messa
          * Fix: inconsistency subscription may lead to consumer miss messages.
          */
         SubscriptionData subscriptionData = this.subscriptionInner.get(topic);
-        long newVersion = System.currentTimeMillis();

Review Comment:
   This is also a good way, but I think using the increment method is simpler. Both have their own advantages.



-- 
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] mxsm closed pull request #6680: [ISSUE #6679]SubscriptionData may not be updated when the client's system time is rolled back

Posted by "mxsm (via GitHub)" <gi...@apache.org>.
mxsm closed pull request #6680: [ISSUE #6679]SubscriptionData may not be updated when the client's system time is rolled back
URL: https://github.com/apache/rocketmq/pull/6680


-- 
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] ferrirW commented on a diff in pull request #6680: [ISSUE #6679]SubscriptionData may not be updated when the client's system time is rolled back

Posted by "ferrirW (via GitHub)" <gi...@apache.org>.
ferrirW commented on code in PR #6680:
URL: https://github.com/apache/rocketmq/pull/6680#discussion_r1184807789


##########
client/src/main/java/org/apache/rocketmq/client/impl/consumer/RebalancePushImpl.java:
##########
@@ -57,7 +57,8 @@ public void messageQueueChanged(String topic, Set<MessageQueue> mqAll, Set<Messa
          * Fix: inconsistency subscription may lead to consumer miss messages.
          */
         SubscriptionData subscriptionData = this.subscriptionInner.get(topic);
-        long newVersion = System.currentTimeMillis();

Review Comment:
   Maybe it could be nanotime



-- 
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] caigy commented on a diff in pull request #6680: [ISSUE #6679]SubscriptionData may not be updated when the client's system time is rolled back

Posted by "caigy (via GitHub)" <gi...@apache.org>.
caigy commented on code in PR #6680:
URL: https://github.com/apache/rocketmq/pull/6680#discussion_r1188040113


##########
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/heartbeat/SubscriptionData.java:
##########
@@ -38,6 +38,9 @@ public class SubscriptionData implements Comparable<SubscriptionData> {
     @JSONField(serialize = false)
     private String filterClassSource;
 
+    @JSONField(serialize = false)
+    private transient long lastUpdateTime = System.currentTimeMillis();

Review Comment:
   Should we use nanoTime?



-- 
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] mxsm commented on pull request #6680: [ISSUE #6679]SubscriptionData may not be updated when the client's system time is rolled back

Posted by "mxsm (via GitHub)" <gi...@apache.org>.
mxsm commented on PR #6680:
URL: https://github.com/apache/rocketmq/pull/6680#issuecomment-1538577390

   > Is there a possibility that introducing changes to Subversion will disrupt other functions?
   
   1. The generation of subversion only made changes to the client side, and no changes were made to the broker side. 
   2. The expiration and removal of SubscriptionData is handled through the newly added lastUpdateTime.


-- 
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] drpmma commented on pull request #6680: [ISSUE #6679]SubscriptionData may not be updated when the client's system time is rolled back

Posted by "drpmma (via GitHub)" <gi...@apache.org>.
drpmma commented on PR #6680:
URL: https://github.com/apache/rocketmq/pull/6680#issuecomment-1537810121

   Is there a possibility that introducing changes to Subversion will disrupt other functions?


-- 
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] mxsm commented on a diff in pull request #6680: [ISSUE #6679]SubscriptionData may not be updated when the client's system time is rolled back

Posted by "mxsm (via GitHub)" <gi...@apache.org>.
mxsm commented on code in PR #6680:
URL: https://github.com/apache/rocketmq/pull/6680#discussion_r1188117819


##########
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/heartbeat/SubscriptionData.java:
##########
@@ -38,6 +38,9 @@ public class SubscriptionData implements Comparable<SubscriptionData> {
     @JSONField(serialize = false)
     private String filterClassSource;
 
+    @JSONField(serialize = false)
+    private transient long lastUpdateTime = System.currentTimeMillis();

Review Comment:
   I don’t think it’s necessary. The lastUpdateTime is generated on the server where the Broker is located. Even if the client generates it, it will not be serialized and sent to the Broker, so there is no problem with 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: commits-unsubscribe@rocketmq.apache.org

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

[GitHub] [rocketmq] ni-ze commented on pull request #6680: [ISSUE #6679]SubscriptionData may not be updated when the client's system time is rolled back

Posted by "ni-ze (via GitHub)" <gi...@apache.org>.
ni-ze commented on PR #6680:
URL: https://github.com/apache/rocketmq/pull/6680#issuecomment-1545044522

   ![image](https://github.com/apache/rocketmq/assets/31175234/15ccb6a2-6189-4c66-8849-165a673a9417)
   Heatbeat does not update the timestamp of subscriptionData. It updated at pullRequest. so the consumer group will not be removed in removeExpireConsumerGroupInfo. The above modification is correct, IMO.


-- 
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