You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by Jaskey <gi...@git.apache.org> on 2017/04/11 11:58:01 UTC
[GitHub] incubator-rocketmq pull request #90: [ROCKETMQ-172]log improvement for rocke...
GitHub user Jaskey opened a pull request:
https://github.com/apache/incubator-rocketmq/pull/90
[ROCKETMQ-172]log improvement for rocketmq client
JIRA: https://issues.apache.org/jira/browse/ROCKETMQ-172?jql=project%20%3D%20ROCKETMQ
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Jaskey/incubator-rocketmq ROCKETMQ-172-log-improvement
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/90.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #90
----
commit 633ac28b8abc63e388d6ebe6081c41371b69ed29
Author: Jaskey <li...@gmail.com>
Date: 2017-04-11T11:54:26Z
improve error message when pull exception
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #90: [ROCKETMQ-172]log improvement for rocketmq cli...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/90
[](https://coveralls.io/builds/11085446)
Coverage increased (+2.8%) to 34.604% when pulling **c46e4c6e8bccaee3aeee2202fa04db378563f9f3 on Jaskey:ROCKETMQ-172-log-improvement** into **a8fa05e8c0b0e57b6e2278747a5f6a8111447a8e on apache:develop**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #90: [ROCKETMQ-172]log improvement for rocketmq cli...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/90
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #90: [ROCKETMQ-172]log improvement for rocke...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/90#discussion_r111699977
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java ---
@@ -577,12 +577,12 @@ public void operationComplete(ResponseFuture responseFuture) {
}
} else {
if (!responseFuture.isSendRequestOK()) {
- pullCallback.onException(new MQClientException("send request failed", responseFuture.getCause()));
+ pullCallback.onException(new MQClientException("send request failed to " + addr + ".request: " + request, responseFuture.getCause()));
} else if (responseFuture.isTimeout()) {
- pullCallback.onException(new MQClientException("wait response timeout " + responseFuture.getTimeoutMillis() + "ms",
+ pullCallback.onException(new MQClientException("wait response from " + addr + " timeout :" + responseFuture.getTimeoutMillis() + "ms" + " request: " + request,
--- End diff --
" request: " -> ", request: "
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #90: [ROCKETMQ-172]log improvement for rocketmq cli...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/90
[](https://coveralls.io/builds/11085103)
Coverage increased (+2.8%) to 34.664% when pulling **790924535b168cc88b5b064c9aa2722aed8b4fee on Jaskey:ROCKETMQ-172-log-improvement** into **a8fa05e8c0b0e57b6e2278747a5f6a8111447a8e on apache:develop**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #90: [ROCKETMQ-172]log improvement for rocketmq cli...
Posted by vesense <gi...@git.apache.org>.
Github user vesense commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/90
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #90: [ROCKETMQ-172]log improvement for rocketmq cli...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/90
@dongeforever @vongosling @shroman
Thank you guys!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #90: [ROCKETMQ-172]log improvement for rocke...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/90#discussion_r111700022
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java ---
@@ -577,12 +577,12 @@ public void operationComplete(ResponseFuture responseFuture) {
}
} else {
if (!responseFuture.isSendRequestOK()) {
- pullCallback.onException(new MQClientException("send request failed", responseFuture.getCause()));
+ pullCallback.onException(new MQClientException("send request failed to " + addr + ".request: " + request, responseFuture.getCause()));
--- End diff --
Or ".request: " -> ". Request: "
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #90: [ROCKETMQ-172]log improvement for rocke...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/90#discussion_r111701802
--- Diff: remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingClient.java ---
@@ -321,6 +321,7 @@ public void updateNameServerAddressList(List<String> addrs) {
if (update) {
Collections.shuffle(addrs);
+ log.info("name server address updated. NEW : {} , OLD: {}",addrs,old);
--- End diff --
just a suggestion to make it prettier ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #90: [ROCKETMQ-172]log improvement for rocke...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/90#discussion_r111701497
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java ---
@@ -577,12 +577,12 @@ public void operationComplete(ResponseFuture responseFuture) {
}
} else {
if (!responseFuture.isSendRequestOK()) {
- pullCallback.onException(new MQClientException("send request failed", responseFuture.getCause()));
+ pullCallback.onException(new MQClientException("send request failed to " + addr + ".request: " + request, responseFuture.getCause()));
--- End diff --
I would modify the typo , your NO2. advice
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #90: [ROCKETMQ-172]log improvement for rocketmq cli...
Posted by dongeforever <gi...@git.apache.org>.
Github user dongeforever commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/90
@Jaskey This PR has been merged
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #90: [ROCKETMQ-172]log improvement for rocketmq cli...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/90
[](https://coveralls.io/builds/11107831)
Coverage increased (+2.7%) to 34.492% when pulling **1e8b575530f32bdb1fac17c906760b947385517d on Jaskey:ROCKETMQ-172-log-improvement** into **a8fa05e8c0b0e57b6e2278747a5f6a8111447a8e on apache:develop**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #90: [ROCKETMQ-172]log improvement for rocke...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/90#discussion_r111316295
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java ---
@@ -577,12 +577,12 @@ public void operationComplete(ResponseFuture responseFuture) {
}
} else {
if (!responseFuture.isSendRequestOK()) {
- pullCallback.onException(new MQClientException("send request failed", responseFuture.getCause()));
--- End diff --
Please replace {} with +
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #90: [ROCKETMQ-172]log improvement for rocketmq cli...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/90
[](https://coveralls.io/builds/11106460)
Coverage increased (+2.7%) to 34.586% when pulling **ec7c8cdf0581f8ff51ba0e27dc1cbbde9fefdfb9 on Jaskey:ROCKETMQ-172-log-improvement** into **a8fa05e8c0b0e57b6e2278747a5f6a8111447a8e on apache:develop**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #90: [ROCKETMQ-172]log improvement for rocke...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/90#discussion_r111700110
--- Diff: remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingClient.java ---
@@ -321,6 +321,7 @@ public void updateNameServerAddressList(List<String> addrs) {
if (update) {
Collections.shuffle(addrs);
+ log.info("name server address updated. NEW : {} , OLD: {}",addrs,old);
--- End diff --
NEW, OLD look ugly to me :)
How about
```
log.info("name server address updated from [{}] to [{}]", addrs, old);
```
?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #90: [ROCKETMQ-172]log improvement for rocketmq cli...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/90
[](https://coveralls.io/builds/11033662)
Coverage increased (+0.01%) to 31.855% when pulling **633ac28b8abc63e388d6ebe6081c41371b69ed29 on Jaskey:ROCKETMQ-172-log-improvement** into **a8fa05e8c0b0e57b6e2278747a5f6a8111447a8e on apache:develop**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #90: [ROCKETMQ-172]log improvement for rocke...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/90#discussion_r111720177
--- Diff: remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingClient.java ---
@@ -321,6 +321,7 @@ public void updateNameServerAddressList(List<String> addrs) {
if (update) {
Collections.shuffle(addrs);
+ log.info("name server address updated. NEW : {} , OLD: {}",addrs,old);
--- End diff --
Perhaps we can post another PR for some logging format, currently I would just retain the current way
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #90: [ROCKETMQ-172]log improvement for rocketmq cli...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/90
@shroman Please review again
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #90: [ROCKETMQ-172]log improvement for rocke...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/90#discussion_r111701589
--- Diff: remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingClient.java ---
@@ -321,6 +321,7 @@ public void updateNameServerAddressList(List<String> addrs) {
if (update) {
Collections.shuffle(addrs);
+ log.info("name server address updated. NEW : {} , OLD: {}",addrs,old);
--- End diff --
@shroman , but OLD , NEW has been already exists in rocketmq's log. please see ConsumerGroupInfo.java for one sample.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #90: [ROCKETMQ-172]log improvement for rocke...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/90#discussion_r111552422
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java ---
@@ -577,12 +577,12 @@ public void operationComplete(ResponseFuture responseFuture) {
}
} else {
if (!responseFuture.isSendRequestOK()) {
- pullCallback.onException(new MQClientException("send request failed", responseFuture.getCause()));
--- End diff --
But this is not a sl4j 's logging, it's a exception, so direct string is needed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #90: [ROCKETMQ-172]log improvement for rocke...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey closed the pull request at:
https://github.com/apache/incubator-rocketmq/pull/90
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #90: [ROCKETMQ-172]log improvement for rocke...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/90#discussion_r111699928
--- Diff: client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java ---
@@ -577,12 +577,12 @@ public void operationComplete(ResponseFuture responseFuture) {
}
} else {
if (!responseFuture.isSendRequestOK()) {
- pullCallback.onException(new MQClientException("send request failed", responseFuture.getCause()));
+ pullCallback.onException(new MQClientException("send request failed to " + addr + ".request: " + request, responseFuture.getCause()));
--- End diff --
How about writing it this way?
`new MQClientException("send request [" + request+ "] failed to " + addr, responseFuture.getCause())`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #90: [ROCKETMQ-172]log improvement for rocketmq cli...
Posted by dongeforever <gi...@git.apache.org>.
Github user dongeforever commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/90
LGTM +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---