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/05/20 01:55:32 UTC

[GitHub] [rocketmq] XiaoyiPeng opened a new pull request, #4350: [ISSUE #4349]: Fix the potential 'IndexOutOfBoundsException' etc

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

   **Make sure set the target branch to `develop`**
   
   ## What is the purpose of the change
   
   For #4349 
   
   ## Brief changelog
   
   - Fix the potential `IndexOutOfBoundsException`
   - `queueId` may be a negative value, fix it.
   
   ## Verifying this change
   
   XXXX
   
   


-- 
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] XiaoyiPeng commented on pull request #4350: [ISSUE #4349]: Fix the potential 'IndexOutOfBoundsException' etc

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

   Hi, @caigy :
   
   I noticed that this issue has been solved in another PR #4447.  I will close 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: dev-unsubscribe@rocketmq.apache.org

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

[GitHub] [rocketmq] XiaoyiPeng commented on a diff in pull request #4350: [ISSUE #4349]: Fix the potential 'IndexOutOfBoundsException' etc

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


##########
client/src/main/java/org/apache/rocketmq/client/latency/LatencyFaultToleranceImpl.java:
##########
@@ -76,7 +76,9 @@ public String pickOneAtLeast() {
             if (half <= 0) {
                 return tmpList.get(0).getName();
             } else {
-                final int i = this.whichItemWorst.incrementAndGet() % half;
+                int i = this.whichItemWorst.incrementAndGet() % half;
+                if (i < 0)

Review Comment:
   Well, I didn't make it clear in the context.
   
    `i` is  return from the method `ThreadLocalIndex#incrementAndGet()`, when variable `index` in this method reach to Integer.MAX_VALUE,  `++index`  will be value of `Integer.MIN_VALUE`,  its absolute value (`Math.abs(Integer.MIN_VALUE)`) is itself, that's a negative value.



-- 
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] Oliverwqcwrw commented on a diff in pull request #4350: [ISSUE #4349]: Fix the potential 'IndexOutOfBoundsException' etc

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


##########
client/src/main/java/org/apache/rocketmq/client/latency/LatencyFaultToleranceImpl.java:
##########
@@ -76,7 +76,9 @@ public String pickOneAtLeast() {
             if (half <= 0) {
                 return tmpList.get(0).getName();
             } else {
-                final int i = this.whichItemWorst.incrementAndGet() % half;
+                int i = this.whichItemWorst.incrementAndGet() % half;
+                if (i < 0)

Review Comment:
   IMO,  `i` can not less than zero



-- 
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] HScarb commented on pull request #4350: [ISSUE #4349]: Fix the potential 'IndexOutOfBoundsException' etc

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

   Good catch! IMO its better to modify `incrementAdnGet` method.


-- 
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] XiaoyiPeng closed pull request #4350: [ISSUE #4349]: Fix the potential 'IndexOutOfBoundsException' etc

Posted by GitBox <gi...@apache.org>.
XiaoyiPeng closed pull request #4350: [ISSUE #4349]: Fix the potential 'IndexOutOfBoundsException' etc
URL: https://github.com/apache/rocketmq/pull/4350


-- 
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] XiaoyiPeng commented on pull request #4350: [ISSUE #4349]: Fix the potential 'IndexOutOfBoundsException' etc

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

   
   
   
   
   > I think it would be better to change `ThreadLocalIndex#incrementAndGet()`: if `index` reaches `Integer.MAX_VALUE`, set it to 0 instead of incrementing it.
   
   OK, Thanks for your review,sir.
    I will check  whether this solution is compatible with the current usage of `ThreadLocalIndex#incrementAndGet() `.


-- 
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] panzhi33 commented on pull request #4350: [ISSUE #4349]: Fix the potential 'IndexOutOfBoundsException' etc

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

   > @ni-ze , @HScarb ,
   > 
   > Thanks for your review. But IMO, For code consistency, I recommend not changing this method `ThreadLocalIndex#incrementAndGet()`, because this method is used in many places. In most scenarios, it judge whether the result returned is positive or negative after the call, as shown below:
   > 
   > ![image](https://user-images.githubusercontent.com/8653312/169936803-9ed90df7-35ff-4b8e-8c3c-225c0cee838f.png)
   > 
   > And if we changing this method(`ThreadLocalIndex#incrementAndGet()`), The variable `index` it returned may be a positive number(eg: `Integer.MAX_VALUE`). when `Math.abs(++index)` is called, we still need to judge if this value is negative, just looks like in the screenshot.
   > 
   > Hi @panzhi33 , @RongtongJin what's your opinion?
   
   I think it is better to modify org.apache.rocketmq.client.common.ThreadLocalIndex#incrementAndGet.


-- 
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] coveralls commented on pull request #4350: [ISSUE #4349]: Fix the potential 'IndexOutOfBoundsException' etc

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

   
   [![Coverage Status](https://coveralls.io/builds/49296967/badge)](https://coveralls.io/builds/49296967)
   
   Coverage decreased (-0.001%) to 52.147% when pulling **28c58d3ee9d2001fbff3e141dd5b43385f40f778 on XiaoyiPeng:issue#4349** into **82a48be00a6d9def14abb871a40cb68cf8a041e9 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.

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

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

[GitHub] [rocketmq] coveralls commented on pull request #4350: [ISSUE #4349]: Fix the potential 'IndexOutOfBoundsException' etc

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

   
   [![Coverage Status](https://coveralls.io/builds/49296967/badge)](https://coveralls.io/builds/49296967)
   
   Coverage decreased (-0.001%) to 52.147% when pulling **28c58d3ee9d2001fbff3e141dd5b43385f40f778 on XiaoyiPeng:issue#4349** into **82a48be00a6d9def14abb871a40cb68cf8a041e9 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.

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 #4350: [ISSUE #4349]: Fix the potential 'IndexOutOfBoundsException' etc

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

   # [Codecov](https://codecov.io/gh/apache/rocketmq/pull/4350?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 [#4350](https://codecov.io/gh/apache/rocketmq/pull/4350?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (28c58d3) into [develop](https://codecov.io/gh/apache/rocketmq/commit/82a48be00a6d9def14abb871a40cb68cf8a041e9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (82a48be) will **decrease** coverage by `0.10%`.
   > The diff coverage is `14.28%`.
   
   ```diff
   @@              Coverage Diff              @@
   ##             develop    #4350      +/-   ##
   =============================================
   - Coverage      48.18%   48.08%   -0.11%     
   + Complexity      5082     5067      -15     
   =============================================
     Files            642      642              
     Lines          42770    42775       +5     
     Branches        5597     5599       +2     
   =============================================
   - Hits           20609    20567      -42     
   - Misses         19655    19688      +33     
   - Partials        2506     2520      +14     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/rocketmq/pull/4350?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...pache/rocketmq/client/latency/MQFaultStrategy.java](https://codecov.io/gh/apache/rocketmq/pull/4350/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvbGF0ZW5jeS9NUUZhdWx0U3RyYXRlZ3kuamF2YQ==) | `16.32% <0.00%> (-1.07%)` | :arrow_down: |
   | [...etmq/client/latency/LatencyFaultToleranceImpl.java](https://codecov.io/gh/apache/rocketmq/pull/4350/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvbGF0ZW5jeS9MYXRlbmN5RmF1bHRUb2xlcmFuY2VJbXBsLmphdmE=) | `48.10% <33.33%> (-1.25%)` | :arrow_down: |
   | [...va/org/apache/rocketmq/store/FlushDiskWatcher.java](https://codecov.io/gh/apache/rocketmq/pull/4350/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL0ZsdXNoRGlza1dhdGNoZXIuamF2YQ==) | `81.25% <0.00%> (-9.38%)` | :arrow_down: |
   | [...org/apache/rocketmq/common/stats/StatsItemSet.java](https://codecov.io/gh/apache/rocketmq/pull/4350/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-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jb21tb24vc3RhdHMvU3RhdHNJdGVtU2V0LmphdmE=) | `41.79% <0.00%> (-8.96%)` | :arrow_down: |
   | [...org/apache/rocketmq/store/ha/WaitNotifyObject.java](https://codecov.io/gh/apache/rocketmq/pull/4350/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL2hhL1dhaXROb3RpZnlPYmplY3QuamF2YQ==) | `66.07% <0.00%> (-5.36%)` | :arrow_down: |
   | [...ava/org/apache/rocketmq/filter/util/BitsArray.java](https://codecov.io/gh/apache/rocketmq/pull/4350/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-ZmlsdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9maWx0ZXIvdXRpbC9CaXRzQXJyYXkuamF2YQ==) | `59.82% <0.00%> (-2.57%)` | :arrow_down: |
   | [...va/org/apache/rocketmq/logging/inner/Appender.java](https://codecov.io/gh/apache/rocketmq/pull/4350/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-bG9nZ2luZy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvbG9nZ2luZy9pbm5lci9BcHBlbmRlci5qYXZh) | `34.83% <0.00%> (-2.25%)` | :arrow_down: |
   | [...a/org/apache/rocketmq/filter/util/BloomFilter.java](https://codecov.io/gh/apache/rocketmq/pull/4350/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-ZmlsdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9maWx0ZXIvdXRpbC9CbG9vbUZpbHRlci5qYXZh) | `60.43% <0.00%> (-2.20%)` | :arrow_down: |
   | [...mq/client/impl/consumer/RebalanceLitePullImpl.java](https://codecov.io/gh/apache/rocketmq/pull/4350/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9jb25zdW1lci9SZWJhbGFuY2VMaXRlUHVsbEltcGwuamF2YQ==) | `72.05% <0.00%> (-1.48%)` | :arrow_down: |
   | [...a/org/apache/rocketmq/store/StoreStatsService.java](https://codecov.io/gh/apache/rocketmq/pull/4350/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL1N0b3JlU3RhdHNTZXJ2aWNlLmphdmE=) | `36.41% <0.00%> (-1.13%)` | :arrow_down: |
   | ... and [12 more](https://codecov.io/gh/apache/rocketmq/pull/4350/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) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/rocketmq/pull/4350?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/rocketmq/pull/4350?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [82a48be...28c58d3](https://codecov.io/gh/apache/rocketmq/pull/4350?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] ni-ze commented on pull request #4350: [ISSUE #4349]: Fix the potential 'IndexOutOfBoundsException' etc

Posted by GitBox <gi...@apache.org>.
ni-ze commented on PR #4350:
URL: https://github.com/apache/rocketmq/pull/4350#issuecomment-1134080541

   I think modify this method is better.
   ![image](https://user-images.githubusercontent.com/31175234/169728353-49338e02-d334-4cae-bd73-fe4daa69cc6b.png)
   
   This method want to return a positive value, but fail in above situation.


-- 
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] Oliverwqcwrw commented on a diff in pull request #4350: [ISSUE #4349]: Fix the potential 'IndexOutOfBoundsException' etc

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


##########
client/src/main/java/org/apache/rocketmq/client/latency/LatencyFaultToleranceImpl.java:
##########
@@ -76,7 +76,9 @@ public String pickOneAtLeast() {
             if (half <= 0) {
                 return tmpList.get(0).getName();
             } else {
-                final int i = this.whichItemWorst.incrementAndGet() % half;
+                int i = this.whichItemWorst.incrementAndGet() % half;
+                if (i < 0)

Review Comment:
   Agree with, I think it would be better to add the comment above the `if(i < 0)`,WDYT



-- 
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] XiaoyiPeng commented on pull request #4350: [ISSUE #4349]: Fix the potential 'IndexOutOfBoundsException' etc

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

   @ni-ze , @HScarb , 
   
   Thanks for your review. 
   But IMO,  For code consistency, I recommend not changing this method `ThreadLocalIndex#incrementAndGet()`, because this method is used in many places.
   In most scenarios, it determine whether the result returned is positive or negative after the call, as  shown below: 
   
   ![image](https://user-images.githubusercontent.com/8653312/169936803-9ed90df7-35ff-4b8e-8c3c-225c0cee838f.png)
   
   And it we changing this method(`ThreadLocalIndex#incrementAndGet()`), The variable `index` it returned  may still be a positive number, eg: `Integer.MAX_VALUE`.  when `Math.abs(++index)` is called, we still need to judge if this value is negative, just looks like in the screenshot.
   
   Hi @panzhi33 , @RongtongJin  what's your opinion?
   
   


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