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/01/20 09:10:04 UTC

[GitHub] [rocketmq] wqliang opened a new pull request #1734: [ISSUE #1727] fix RequestCallback executed multi times

wqliang opened a new pull request #1734: [ISSUE #1727] fix RequestCallback executed multi times
URL: https://github.com/apache/rocketmq/pull/1734
 
 
   ## What is the purpose of the change
   To fix problem mentioned in [ISSUE 1727](https://github.com/apache/rocketmq/issues/1727)
   When there is more than one DefaultMQProducerImpl instance in one process, RequestCallback is possible to be executed multi times.
   
   ## Brief changelog
   
   1. move  `DefaultMQProducerImpl.timer` to `RequestFutureTable.timer` to avoid multi timer scan the future table;
   2. add flag to distinguish whether a ReqeustCallback has been executed or not;
   
   ## Verifying this change
   
   XXXX
   
   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`.
   
   - [x] Make sure there is a [Github issue](https://github.com/apache/rocketmq/issues) filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue. 
   - [x] Format the pull request title like `[ISSUE #123] Fix UnknownException when host config not exist`. Each commit in the pull request should have a meaningful subject line and body.
   - [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
   - [x] Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in [test module](https://github.com/apache/rocketmq/tree/master/test).
   - [x] Run `mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle` to make sure basic checks pass. Run `mvn clean install -DskipITs` to make sure unit-test pass. Run `mvn clean test-compile failsafe:integration-test`  to make sure integration-test pass.
   - [ ] If this contribution is large, please file an [Apache Individual Contributor License Agreement](http://www.apache.org/licenses/#clas).
   

----------------------------------------------------------------
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 #1734: [ISSUE #1727] fix RequestCallback executed multi times

Posted by GitBox <gi...@apache.org>.
2259289435 commented on issue #1734: [ISSUE #1727] fix RequestCallback executed multi times
URL: https://github.com/apache/rocketmq/pull/1734#issuecomment-576996418
 
 
   > ## What is the purpose of the change
   > To fix problem mentioned in [ISSUE 1727](https://github.com/apache/rocketmq/issues/1727)
   > When there is more than one DefaultMQProducerImpl instance in one process, RequestCallback is possible to be executed multi times.
   > 
   > ## Brief changelog
   > 1. move  `DefaultMQProducerImpl.timer` to `RequestFutureTable.timer` to avoid multi timer scan the future table;
   > 2. add flag to distinguish whether a ReqeustCallback has been executed or not;
   > 
   > ## Verifying this change
   > XXXX
   > 
   > 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`.
   > 
   > * [x]  Make sure there is a [Github issue](https://github.com/apache/rocketmq/issues) filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
   > * [x]  Format the pull request title like `[ISSUE #123] Fix UnknownException when host config not exist`. Each commit in the pull request should have a meaningful subject line and body.
   > * [x]  Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
   > * [x]  Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in [test module](https://github.com/apache/rocketmq/tree/master/test).
   > * [x]  Run `mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle` to make sure basic checks pass. Run `mvn clean install -DskipITs` to make sure unit-test pass. Run `mvn clean test-compile failsafe:integration-test`  to make sure integration-test pass.
   > * [ ]  If this contribution is large, please file an [Apache Individual Contributor License Agreement](http://www.apache.org/licenses/#clas).
   
   修改callback有点不好
   1，callback是很早之前就开放给客户端使用了，现在改为只能执行一次，对低版本升级兼容有影响
   2，重复多次只有callback异常才会发生，我建议的是迭代的时候标记key，然后执行exception的时候再去removekey，这样也解决了并发问题，对原有的callback也没有影响。

----------------------------------------------------------------
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] wqliang commented on issue #1734: [ISSUE #1727] fix RequestCallback executed multi times

Posted by GitBox <gi...@apache.org>.
wqliang commented on issue #1734: [ISSUE #1727] fix RequestCallback executed multi times
URL: https://github.com/apache/rocketmq/pull/1734#issuecomment-593098018
 
 
   > @wqliang ci failed, err info: Name 'timer' must match pattern '(^[A-Z][A-Z0-9]_(_[A-Z0-9]+)_$)|(^log$)'.
   
   @xujianhai666 Got it, I have corrected it. Thanks.

----------------------------------------------------------------
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 removed a comment on issue #1734: [ISSUE #1727] fix RequestCallback executed multi times

Posted by GitBox <gi...@apache.org>.
2259289435 removed a comment on issue #1734: [ISSUE #1727] fix RequestCallback executed multi times
URL: https://github.com/apache/rocketmq/pull/1734#issuecomment-576996279
 
 
   修改callback有点不好
   1，callback是很早之前就开放给客户端使用了，现在改为只能执行一次，对低版本升级兼容有影响
   2，重复多次只有callback异常才会发生，我建议的是迭代的时候标记key，然后执行exception的时候再去removekey，这样也解决了并发问题，对原有的callback也没有影响。

----------------------------------------------------------------
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] wqliang commented on a change in pull request #1734: [ISSUE #1727] fix RequestCallback executed multi times

Posted by GitBox <gi...@apache.org>.
wqliang commented on a change in pull request #1734: [ISSUE #1727] fix RequestCallback executed multi times
URL: https://github.com/apache/rocketmq/pull/1734#discussion_r386107376
 
 

 ##########
 File path: client/src/main/java/org/apache/rocketmq/client/producer/RequestResponseFuture.java
 ##########
 @@ -39,7 +41,7 @@ public RequestResponseFuture(String correlationId, long timeoutMillis, RequestCa
     }
 
     public void executeRequestCallback() {
-        if (requestCallback != null) {
+        if (requestCallback != null && executeCallbackOnlyOnce.compareAndSet(false, true)) {
 
 Review comment:
   I have pulled the latest code from 'develop' branch and not very sure what you mean here, could you plz give me more details?

----------------------------------------------------------------
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 #1734: [ISSUE #1727] fix RequestCallback executed multi times

Posted by GitBox <gi...@apache.org>.
2259289435 commented on issue #1734: [ISSUE #1727] fix RequestCallback executed multi times
URL: https://github.com/apache/rocketmq/pull/1734#issuecomment-576996279
 
 
   修改callback有点不好
   1，callback是很早之前就开放给客户端使用了，现在改为只能执行一次，对低版本升级兼容有影响
   2，重复多次只有callback异常才会发生，我建议的是迭代的时候标记key，然后执行exception的时候再去removekey，这样也解决了并发问题，对原有的callback也没有影响。

----------------------------------------------------------------
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 #1734: [ISSUE #1727] fix RequestCallback executed multi times

Posted by GitBox <gi...@apache.org>.
coveralls commented on issue #1734: [ISSUE #1727] fix RequestCallback executed multi times
URL: https://github.com/apache/rocketmq/pull/1734#issuecomment-593094997
 
 
   
   [![Coverage Status](https://coveralls.io/builds/29055436/badge)](https://coveralls.io/builds/29055436)
   
   Coverage increased (+0.03%) to 50.904% when pulling **753abde6a572d21bcd9322b94adf4b30367b4a56 on wqliang:fixRequestCallbackExecute** into **997164b1cdc122bfd767d23b476a789461507332 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] 2259289435 commented on issue #1734: [ISSUE #1727] fix RequestCallback executed multi times

Posted by GitBox <gi...@apache.org>.
2259289435 commented on issue #1734: [ISSUE #1727] fix RequestCallback executed multi times
URL: https://github.com/apache/rocketmq/pull/1734#issuecomment-576998282
 
 
   修改callback有点不好
   1，callback是很早之前就开放给客户端使用了，现在改为只能执行一次，对低版本升级兼容有影响
   2，重复多次只有callback异常才会发生，我建议的是迭代的时候标记key，然后执行exception的时候再去removekey，这样也解决了并发问题，对原有的callback也没有影响。
   
   Modifying the callback is a bit of a bad thing. 
   1. Callback was opened to the client a long time ago, but now it can only be executed once, which will affect the compatibility of lower version upgrade. 
   2. Only the callback exception will occur if the callback is repeated several times. What I suggest is to mark the key during iteration and then removekey when the exception is executed, which also solves the concurrency problem and has no effect on the original callback.
   
   public static void scanExpiredRequest() {
           Iterator<Map.Entry<String, RequestResponseFuture>> it = requestFutureTable.entrySet().iterator();
           Set<String> expiredKeys = new HashSet<String>()
           while (it.hasNext()) {
               Map.Entry<String, RequestResponseFuture> next = it.next();
               RequestResponseFuture rep = next.getValue();
   
               if (rep.isTimeout()) {
                   expiredKeys .add(next.getKey());
               }
           }
           for(String expiredKey : expiredKeys){
                   RequestResponseFuture rf = requestFutureTable.remove(expiredKey);
                   if(rf != null){
                           try {
                                   Throwable cause = new RequestTimeoutException(ClientErrorCode.REQUEST_TIMEOUT_EXCEPTION, "request timeout, no reply message.");
                                   rf.setCause(cause);
                                   rf.executeRequestCallback();
                           } catch (Throwable e) {
                               log.warn("scanResponseTable, operationComplete Exception", e);
                           }
                   }
           }
       }
   
   
   
   
   2259289435@qq.com
    
   From: wqliang
   Date: 2020-01-20 17:10
   To: apache/rocketmq
   CC: Subscribed
   Subject: [apache/rocketmq] [ISSUE #1727] fix RequestCallback executed multi times (#1734)
   What is the purpose of the change
   To fix problem mentioned in ISSUE 1727
   When there is more than one DefaultMQProducerImpl instance in one process, RequestCallback is possible to be executed multi times.
   Brief changelog
   move DefaultMQProducerImpl.timer to RequestFutureTable.timer to avoid multi timer scan the future table;
   add flag to distinguish whether a ReqeustCallback has been executed or not;
   Verifying this change
   XXXX
   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.
   Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
   Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
   Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
   Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
   Run mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.
   If this contribution is large, please file an Apache Individual Contributor License Agreement.
   
   
   You can view, comment on, or merge this pull request online at:
     https://github.com/apache/rocketmq/pull/1734
   Commit Summary
   fix RequestCallback executed multi times
   File Changes
   M client/src/main/java/org/apache/rocketmq/client/impl/ClientRemotingProcessor.java (2) 
   M client/src/main/java/org/apache/rocketmq/client/impl/producer/DefaultMQProducerImpl.java (14) 
   M client/src/main/java/org/apache/rocketmq/client/producer/RequestFutureTable.java (16) 
   M client/src/main/java/org/apache/rocketmq/client/producer/RequestResponseFuture.java (4) 
   M client/src/test/java/org/apache/rocketmq/client/producer/DefaultMQProducerTest.java (9) 
   M client/src/test/java/org/apache/rocketmq/client/producer/RequestResponseFutureTest.java (15) 
   Patch Links:
   https://github.com/apache/rocketmq/pull/1734.patch
   https://github.com/apache/rocketmq/pull/1734.diff
   —
   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] xujianhai666 commented on issue #1734: [ISSUE #1727] fix RequestCallback executed multi times

Posted by GitBox <gi...@apache.org>.
xujianhai666 commented on issue #1734: [ISSUE #1727] fix RequestCallback executed multi times
URL: https://github.com/apache/rocketmq/pull/1734#issuecomment-592944544
 
 
   @wqliang  ci failed, err info: Name 'timer' must match pattern '(^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$)|(^log$)'.

----------------------------------------------------------------
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 removed a comment on issue #1734: [ISSUE #1727] fix RequestCallback executed multi times

Posted by GitBox <gi...@apache.org>.
2259289435 removed a comment on issue #1734: [ISSUE #1727] fix RequestCallback executed multi times
URL: https://github.com/apache/rocketmq/pull/1734#issuecomment-576996418
 
 
   > ## What is the purpose of the change
   > To fix problem mentioned in [ISSUE 1727](https://github.com/apache/rocketmq/issues/1727)
   > When there is more than one DefaultMQProducerImpl instance in one process, RequestCallback is possible to be executed multi times.
   > 
   > ## Brief changelog
   > 1. move  `DefaultMQProducerImpl.timer` to `RequestFutureTable.timer` to avoid multi timer scan the future table;
   > 2. add flag to distinguish whether a ReqeustCallback has been executed or not;
   > 
   > ## Verifying this change
   > XXXX
   > 
   > 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`.
   > 
   > * [x]  Make sure there is a [Github issue](https://github.com/apache/rocketmq/issues) filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
   > * [x]  Format the pull request title like `[ISSUE #123] Fix UnknownException when host config not exist`. Each commit in the pull request should have a meaningful subject line and body.
   > * [x]  Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
   > * [x]  Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in [test module](https://github.com/apache/rocketmq/tree/master/test).
   > * [x]  Run `mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle` to make sure basic checks pass. Run `mvn clean install -DskipITs` to make sure unit-test pass. Run `mvn clean test-compile failsafe:integration-test`  to make sure integration-test pass.
   > * [ ]  If this contribution is large, please file an [Apache Individual Contributor License Agreement](http://www.apache.org/licenses/#clas).
   
   修改callback有点不好
   1，callback是很早之前就开放给客户端使用了，现在改为只能执行一次，对低版本升级兼容有影响
   2，重复多次只有callback异常才会发生，我建议的是迭代的时候标记key，然后执行exception的时候再去removekey，这样也解决了并发问题，对原有的callback也没有影响。

----------------------------------------------------------------
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] xujianhai666 commented on a change in pull request #1734: [ISSUE #1727] fix RequestCallback executed multi times

Posted by GitBox <gi...@apache.org>.
xujianhai666 commented on a change in pull request #1734: [ISSUE #1727] fix RequestCallback executed multi times
URL: https://github.com/apache/rocketmq/pull/1734#discussion_r386027093
 
 

 ##########
 File path: client/src/main/java/org/apache/rocketmq/client/producer/RequestResponseFuture.java
 ##########
 @@ -39,7 +41,7 @@ public RequestResponseFuture(String correlationId, long timeoutMillis, RequestCa
     }
 
     public void executeRequestCallback() {
-        if (requestCallback != null) {
+        if (requestCallback != null && executeCallbackOnlyOnce.compareAndSet(false, true)) {
 
 Review comment:
   code has change. see latest code: `if (sendRequestOk && cause == null) {`

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