Re: [PR] [ISSUE #8168] fix: There's no need to retry when async produce already timeout [rocketmq]
lizhimins merged PR #8169: URL: https://github.com/apache/rocketmq/pull/8169 -- 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-unsubscr...@rocketmq.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [ISSUE #8168]fix: There's no need to retry when async produce already timeout [rocketmq]
guyinyou commented on PR #8169: URL: https://github.com/apache/rocketmq/pull/8169#issuecomment-2123737540 lgtm -- 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-unsubscr...@rocketmq.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [ISSUE #8168]fix: There's no need to retry when async produce already timeout [rocketmq]
humkum commented on code in PR #8169:
URL: https://github.com/apache/rocketmq/pull/8169#discussion_r1609156131
##
client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java:
##
@@ -704,7 +704,7 @@ public void operationFail(Throwable throwable) {
onExceptionImpl(brokerName, msg, timeoutMillis - cost,
request, sendCallback, topicPublishInfo, instance,
retryTimesWhenSendFailed, times, ex, context,
true, producer);
} else {
-MQClientException ex = new MQClientException("unknow
reseaon", throwable);
+MQClientException ex = new MQClientException("unknown
reason", throwable);
Review Comment:
You are right, and the invokeAsync won't throw
RemotingTooMuchRequestException, I will make some 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-unsubscr...@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [ISSUE #8168]fix: There's no need to retry when async produce already timeout [rocketmq]
Willhow-Gao commented on code in PR #8169:
URL: https://github.com/apache/rocketmq/pull/8169#discussion_r1608477337
##
client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java:
##
@@ -704,7 +704,7 @@ public void operationFail(Throwable throwable) {
onExceptionImpl(brokerName, msg, timeoutMillis - cost,
request, sendCallback, topicPublishInfo, instance,
retryTimesWhenSendFailed, times, ex, context,
true, producer);
} else {
-MQClientException ex = new MQClientException("unknow
reseaon", throwable);
+MQClientException ex = new MQClientException("unknown
reason", throwable);
Review Comment:
If the exception here is a RemotingTooMuchRequestException, you should also
stop retrying.
--
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-unsubscr...@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [ISSUE #8168]fix: There's no need to retry when async produce already timeout [rocketmq]
RongtongJin commented on PR #8169: URL: https://github.com/apache/rocketmq/pull/8169#issuecomment-2122614614 PTAL～@guyinyou -- 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-unsubscr...@rocketmq.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [ISSUE #8168]fix: There's no need to retry when async produce already timeout [rocketmq]
humkum commented on code in PR #8169:
URL: https://github.com/apache/rocketmq/pull/8169#discussion_r1607562676
##
client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java:
##
@@ -713,8 +713,10 @@ public void operationFail(Throwable throwable) {
} catch (Exception ex) {
long cost = System.currentTimeMillis() - beginStartTime;
producer.updateFaultItem(brokerName, cost, true, false);
-onExceptionImpl(brokerName, msg, timeoutMillis - cost, request,
sendCallback, topicPublishInfo, instance,
-retryTimesWhenSendFailed, times, ex, context, true, producer);
+if (!(ex instanceof RemotingTooMuchRequestException)) {
+onExceptionImpl(brokerName, msg, timeoutMillis - cost,
request, sendCallback, topicPublishInfo, instance,
+retryTimesWhenSendFailed, times, ex, context, true,
producer);
+}
Review Comment:
Got，I will make some change for this.
--
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-unsubscr...@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [ISSUE #8168]fix: There's no need to retry when async produce already timeout [rocketmq]
RongtongJin commented on code in PR #8169:
URL: https://github.com/apache/rocketmq/pull/8169#discussion_r1607439114
##
client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java:
##
@@ -713,8 +713,10 @@ public void operationFail(Throwable throwable) {
} catch (Exception ex) {
long cost = System.currentTimeMillis() - beginStartTime;
producer.updateFaultItem(brokerName, cost, true, false);
-onExceptionImpl(brokerName, msg, timeoutMillis - cost, request,
sendCallback, topicPublishInfo, instance,
-retryTimesWhenSendFailed, times, ex, context, true, producer);
+if (!(ex instanceof RemotingTooMuchRequestException)) {
+onExceptionImpl(brokerName, msg, timeoutMillis - cost,
request, sendCallback, topicPublishInfo, instance,
+retryTimesWhenSendFailed, times, ex, context, true,
producer);
+}
Review Comment:
Even if not retried, the onExceptionImpl method should still be executed,
with the retry parameter set to false.
--
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-unsubscr...@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [ISSUE #8168]fix: There's no need to retry when async produce already timeout [rocketmq]
codecov-commenter commented on PR #8169: URL: https://github.com/apache/rocketmq/pull/8169#issuecomment-2119993482 ## [Codecov](https://app.codecov.io/gh/apache/rocketmq/pull/8169?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report Attention: Patch coverage is `33.3%` with `2 lines` in your changes are missing coverage. Please review. > Project coverage is 42.87%. Comparing base [(`0ad0244`)](https://app.codecov.io/gh/apache/rocketmq/commit/0ad0244fe504d9e20a15e83222826f1172bdc4b3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`de860d1`)](https://app.codecov.io/gh/apache/rocketmq/pull/8169?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). | [Files](https://app.codecov.io/gh/apache/rocketmq/pull/8169?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines | |---|---|---| | [...g/apache/rocketmq/client/impl/MQClientAPIImpl.java](https://app.codecov.io/gh/apache/rocketmq/pull/8169?src=pr&el=tree&filepath=client%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Frocketmq%2Fclient%2Fimpl%2FMQClientAPIImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9NUUNsaWVudEFQSUltcGwuamF2YQ==) | 33.33% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/rocketmq/pull/8169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop#8169 +/- ## = - Coverage 42.92% 42.87% -0.05% + Complexity 1037210356 -16 = Files 1270 1270 Lines 8869688697 +1 Branches 1140211403 +1 = - Hits 3807238033 -39 - Misses 4592245954 +32 - Partials4702 4710 +8 ``` [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/rocketmq/pull/8169?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). -- 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-unsubscr...@rocketmq.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
