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 2020/02/03 13:28:29 UTC

[GitHub] [rocketmq] rushsky518 opened a new pull request #1758: Fix bug in MessageClientIDSetter

rushsky518 opened a new pull request #1758: Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758
 
 
   pid in msgId is erased

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] coveralls edited a comment on issue #1758: [ISSUE #175]Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on issue #1758: [ISSUE #175]Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#issuecomment-581425021
 
 
   
   [![Coverage Status](https://coveralls.io/builds/28676938/badge)](https://coveralls.io/builds/28676938)
   
   Coverage increased (+0.1%) to 51.012% when pulling **9385825c0edbadb8d4130f39820a91488dd3ff08 on rushsky518:msgid** into **dd943622573ece3bc44fea81c10aa6c42fa6991c on apache:develop**.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] duhenglucky commented on a change in pull request #1758: [ISSUE #1751]Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
duhenglucky commented on a change in pull request #1758: [ISSUE #1751]Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#discussion_r378069245
 
 

 ##########
 File path: common/src/main/java/org/apache/rocketmq/common/message/MessageClientIDSetter.java
 ##########
 @@ -107,6 +104,15 @@ public static String getIPStrFromID(String msgID) {
         return result;
     }
 
+    public static short getPidFromID(String msgID) {
 
 Review comment:
   it seems this method has some code repeat with org.apache.rocketmq.common.message.MessageClientIDSetter#getIPFromID

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] coveralls edited a comment on issue #1758: [ISSUE #175]Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on issue #1758: [ISSUE #175]Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#issuecomment-581425021
 
 
   
   [![Coverage Status](https://coveralls.io/builds/28676549/badge)](https://coveralls.io/builds/28676549)
   
   Coverage increased (+0.03%) to 50.933% when pulling **81a19560cd0491924c3d1b47c01003d7a455f0da on rushsky518:msgid** into **dd943622573ece3bc44fea81c10aa6c42fa6991c on apache:develop**.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] coveralls commented on issue #1758: Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
coveralls commented on issue #1758: Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#issuecomment-581425021
 
 
   
   [![Coverage Status](https://coveralls.io/builds/28497098/badge)](https://coveralls.io/builds/28497098)
   
   Coverage decreased (-0.02%) to 50.881% when pulling **4fb7710f7773fdd184b2a3be96f71be5a4a5fb7b on rushsky518:msgid** into **dd943622573ece3bc44fea81c10aa6c42fa6991c on apache:develop**.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] rushsky518 commented on a change in pull request #1758: [ISSUE #175]Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
rushsky518 commented on a change in pull request #1758: [ISSUE #175]Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#discussion_r378051101
 
 

 ##########
 File path: common/src/main/java/org/apache/rocketmq/common/message/MessageClientIDSetter.java
 ##########
 @@ -37,13 +37,10 @@
         } catch (Exception e) {
             ip = createFakeIP();
         }
-        LEN = ip.length + 2 + 4 + 4 + 2;
-        ByteBuffer tempBuffer = ByteBuffer.allocate(ip.length + 2 + 4);
-        tempBuffer.position(0);
+        LEN = ip.length + 4 + 4 + 4 + 2;
+        ByteBuffer tempBuffer = ByteBuffer.allocate(ip.length + 4 + 4);
         tempBuffer.put(ip);
-        tempBuffer.position(ip.length);
         tempBuffer.putInt(UtilAll.getPid());
-        tempBuffer.position(ip.length + 2);
 
 Review comment:
   and there is a redundant "org.apache.rocketmq.common.MixAll#getPID"

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] 2259289435 commented on issue #1758: [ISSUE #175]Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
2259289435 commented on issue #1758: [ISSUE #175]Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#issuecomment-585042982
 
 
   pid是short值，原先的写法putInt在后退两2，抹掉了低位，导致单机多jvm时pid的隔离效果消失，msgid会产生重复的bug
   
   所以直接putshort（（short）pid）就可以修复这个问题了，后续都不用动，也符合原先pid 占用 2 的本意。
   不需要修改成占用 4
   
   
   
   2259289435@qq.com
    
   From: rushsky518
   Date: 2020-02-12 13:38
   To: apache/rocketmq
   CC: Subscribed
   Subject: Re: [apache/rocketmq] Fix bug in MessageClientIDSetter (#1758)
   @rushsky518 commented on this pull request.
   
   
   In common/src/main/java/org/apache/rocketmq/common/message/MessageClientIDSetter.java:
   >          tempBuffer.putInt(UtilAll.getPid());
   -        tempBuffer.position(ip.length + 2);
   
   issue is here：#1751
   —
   You are receiving this because you are subscribed to this thread.
   Reply to this email directly, view it on GitHub, or unsubscribe.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] rushsky518 removed a comment on issue #1758: Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
rushsky518 removed a comment on issue #1758: Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#issuecomment-585037466
 
 
   > @rushsky518 thanks for your contribution, this bug will cause MsgId repeat, and could you pull an issue first? we will merge this PR ASAP.
   
   https://github.com/apache/rocketmq/issues/1751

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] duhenglucky commented on issue #1758: Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
duhenglucky commented on issue #1758: Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#issuecomment-585013445
 
 
   @rushsky518 thanks for your contribution, this bug will cause MsgId repeat, and could you pull an issue first? we will merge this PR ASAP.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] rushsky518 commented on a change in pull request #1758: Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
rushsky518 commented on a change in pull request #1758: Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#discussion_r378040582
 
 

 ##########
 File path: common/src/main/java/org/apache/rocketmq/common/message/MessageClientIDSetter.java
 ##########
 @@ -37,13 +37,10 @@
         } catch (Exception e) {
             ip = createFakeIP();
         }
-        LEN = ip.length + 2 + 4 + 4 + 2;
-        ByteBuffer tempBuffer = ByteBuffer.allocate(ip.length + 2 + 4);
-        tempBuffer.position(0);
+        LEN = ip.length + 4 + 4 + 4 + 2;
+        ByteBuffer tempBuffer = ByteBuffer.allocate(ip.length + 4 + 4);
         tempBuffer.put(ip);
-        tempBuffer.position(ip.length);
         tempBuffer.putInt(UtilAll.getPid());
-        tempBuffer.position(ip.length + 2);
 
 Review comment:
   "The pid_t data type is a signed integer"
   https://www.gnu.org/software/libc/manual/html_node/Process-Identification.html

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] duhenglucky commented on a change in pull request #1758: Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
duhenglucky commented on a change in pull request #1758: Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#discussion_r378026223
 
 

 ##########
 File path: common/src/main/java/org/apache/rocketmq/common/message/MessageClientIDSetter.java
 ##########
 @@ -37,13 +37,10 @@
         } catch (Exception e) {
             ip = createFakeIP();
         }
-        LEN = ip.length + 2 + 4 + 4 + 2;
-        ByteBuffer tempBuffer = ByteBuffer.allocate(ip.length + 2 + 4);
-        tempBuffer.position(0);
+        LEN = ip.length + 4 + 4 + 4 + 2;
+        ByteBuffer tempBuffer = ByteBuffer.allocate(ip.length + 4 + 4);
         tempBuffer.put(ip);
-        tempBuffer.position(ip.length);
         tempBuffer.putInt(UtilAll.getPid());
-        tempBuffer.position(ip.length + 2);
 
 Review comment:
   pid is short，so maybe putShort can resolve this issue，and this indeed a bug, ip.length + 2 will keep the lower 2 bytes. 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] rushsky518 commented on a change in pull request #1758: Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
rushsky518 commented on a change in pull request #1758: Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#discussion_r378040582
 
 

 ##########
 File path: common/src/main/java/org/apache/rocketmq/common/message/MessageClientIDSetter.java
 ##########
 @@ -37,13 +37,10 @@
         } catch (Exception e) {
             ip = createFakeIP();
         }
-        LEN = ip.length + 2 + 4 + 4 + 2;
-        ByteBuffer tempBuffer = ByteBuffer.allocate(ip.length + 2 + 4);
-        tempBuffer.position(0);
+        LEN = ip.length + 4 + 4 + 4 + 2;
+        ByteBuffer tempBuffer = ByteBuffer.allocate(ip.length + 4 + 4);
         tempBuffer.put(ip);
-        tempBuffer.position(ip.length);
         tempBuffer.putInt(UtilAll.getPid());
-        tempBuffer.position(ip.length + 2);
 
 Review comment:
   "The pid_t data type is a signed integer"
   https://www.gnu.org/software/libc/manual/html_node/Process-Identification.html

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] rushsky518 commented on a change in pull request #1758: Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
rushsky518 commented on a change in pull request #1758: Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#discussion_r378047376
 
 

 ##########
 File path: common/src/main/java/org/apache/rocketmq/common/message/MessageClientIDSetter.java
 ##########
 @@ -37,13 +37,10 @@
         } catch (Exception e) {
             ip = createFakeIP();
         }
-        LEN = ip.length + 2 + 4 + 4 + 2;
-        ByteBuffer tempBuffer = ByteBuffer.allocate(ip.length + 2 + 4);
-        tempBuffer.position(0);
+        LEN = ip.length + 4 + 4 + 4 + 2;
+        ByteBuffer tempBuffer = ByteBuffer.allocate(ip.length + 4 + 4);
         tempBuffer.put(ip);
-        tempBuffer.position(ip.length);
         tempBuffer.putInt(UtilAll.getPid());
-        tempBuffer.position(ip.length + 2);
 
 Review comment:
   https://unix.stackexchange.com/questions/16883/what-is-the-maximum-value-of-the-process-id

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] duhenglucky commented on a change in pull request #1758: [ISSUE #1751]Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
duhenglucky commented on a change in pull request #1758: [ISSUE #1751]Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#discussion_r378069245
 
 

 ##########
 File path: common/src/main/java/org/apache/rocketmq/common/message/MessageClientIDSetter.java
 ##########
 @@ -107,6 +104,15 @@ public static String getIPStrFromID(String msgID) {
         return result;
     }
 
+    public static short getPidFromID(String msgID) {
 
 Review comment:
   it seems this method repeat with org.apache.rocketmq.common.message.MessageClientIDSetter#getIPFromID

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] rushsky518 commented on issue #1758: Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
rushsky518 commented on issue #1758: Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#issuecomment-585037466
 
 
   > @rushsky518 thanks for your contribution, this bug will cause MsgId repeat, and could you pull an issue first? we will merge this PR ASAP.
   
   https://github.com/apache/rocketmq/issues/1751

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] duhenglucky commented on a change in pull request #1758: [ISSUE #1751]Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
duhenglucky commented on a change in pull request #1758: [ISSUE #1751]Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#discussion_r378069245
 
 

 ##########
 File path: common/src/main/java/org/apache/rocketmq/common/message/MessageClientIDSetter.java
 ##########
 @@ -107,6 +104,15 @@ public static String getIPStrFromID(String msgID) {
         return result;
     }
 
+    public static short getPidFromID(String msgID) {
 
 Review comment:
   it seems this method has some code repeat with org.apache.rocketmq.common.message.MessageClientIDSetter#getIPFromID

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] coveralls edited a comment on issue #1758: [ISSUE #1751]Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on issue #1758: [ISSUE #1751]Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#issuecomment-581425021
 
 
   
   [![Coverage Status](https://coveralls.io/builds/28677828/badge)](https://coveralls.io/builds/28677828)
   
   Coverage increased (+0.02%) to 50.921% when pulling **5de8764b58f80ce1c3c957852ddad30a747da78a on rushsky518:msgid** into **dd943622573ece3bc44fea81c10aa6c42fa6991c on apache:develop**.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] rushsky518 commented on a change in pull request #1758: [ISSUE #175]Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
rushsky518 commented on a change in pull request #1758: [ISSUE #175]Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#discussion_r378051101
 
 

 ##########
 File path: common/src/main/java/org/apache/rocketmq/common/message/MessageClientIDSetter.java
 ##########
 @@ -37,13 +37,10 @@
         } catch (Exception e) {
             ip = createFakeIP();
         }
-        LEN = ip.length + 2 + 4 + 4 + 2;
-        ByteBuffer tempBuffer = ByteBuffer.allocate(ip.length + 2 + 4);
-        tempBuffer.position(0);
+        LEN = ip.length + 4 + 4 + 4 + 2;
+        ByteBuffer tempBuffer = ByteBuffer.allocate(ip.length + 4 + 4);
         tempBuffer.put(ip);
-        tempBuffer.position(ip.length);
         tempBuffer.putInt(UtilAll.getPid());
-        tempBuffer.position(ip.length + 2);
 
 Review comment:
   and there are 2 methods 
   "org.apache.rocketmq.common.MixAll#getPID"
   "org.apache.rocketmq.common.UtilAll#getPid"

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] rushsky518 commented on a change in pull request #1758: Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
rushsky518 commented on a change in pull request #1758: Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#discussion_r378048610
 
 

 ##########
 File path: common/src/main/java/org/apache/rocketmq/common/message/MessageClientIDSetter.java
 ##########
 @@ -37,13 +37,10 @@
         } catch (Exception e) {
             ip = createFakeIP();
         }
-        LEN = ip.length + 2 + 4 + 4 + 2;
-        ByteBuffer tempBuffer = ByteBuffer.allocate(ip.length + 2 + 4);
-        tempBuffer.position(0);
+        LEN = ip.length + 4 + 4 + 4 + 2;
+        ByteBuffer tempBuffer = ByteBuffer.allocate(ip.length + 4 + 4);
         tempBuffer.put(ip);
-        tempBuffer.position(ip.length);
         tempBuffer.putInt(UtilAll.getPid());
-        tempBuffer.position(ip.length + 2);
 
 Review comment:
   issue is here：https://github.com/apache/rocketmq/issues/1751

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] duhenglucky commented on a change in pull request #1758: Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
duhenglucky commented on a change in pull request #1758: Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758#discussion_r378026223
 
 

 ##########
 File path: common/src/main/java/org/apache/rocketmq/common/message/MessageClientIDSetter.java
 ##########
 @@ -37,13 +37,10 @@
         } catch (Exception e) {
             ip = createFakeIP();
         }
-        LEN = ip.length + 2 + 4 + 4 + 2;
-        ByteBuffer tempBuffer = ByteBuffer.allocate(ip.length + 2 + 4);
-        tempBuffer.position(0);
+        LEN = ip.length + 4 + 4 + 4 + 2;
+        ByteBuffer tempBuffer = ByteBuffer.allocate(ip.length + 4 + 4);
         tempBuffer.put(ip);
-        tempBuffer.position(ip.length);
         tempBuffer.putInt(UtilAll.getPid());
-        tempBuffer.position(ip.length + 2);
 
 Review comment:
   pid is short，so maybe putShort can resolve this issue，and this indeed a bug, ip.length + 2 will remove the lower 2 bytes. 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [rocketmq] duhenglucky merged pull request #1758: [ISSUE #1751]Fix bug in MessageClientIDSetter

Posted by GitBox <gi...@apache.org>.
duhenglucky merged pull request #1758: [ISSUE #1751]Fix bug in MessageClientIDSetter
URL: https://github.com/apache/rocketmq/pull/1758
 
 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services