[ROCKETMQ-172]log improvement for rocketmq client #90

Jaskey · 2017-04-11T11:57:55Z

JIRA: https://issues.apache.org/jira/browse/ROCKETMQ-172?jql=project%20%3D%20ROCKETMQ

This should be a pr which contains many improvement among the client code fragment.

@lizhanhui @shroman @vongosling @zhouxinyu
Please help pointing out more places where log/message should be improved.

coveralls · 2017-04-11T12:16:32Z

Coverage increased (+0.01%) to 31.855% when pulling 633ac28 on Jaskey:ROCKETMQ-172-log-improvement into a8fa05e on apache:develop.

coveralls · 2017-04-11T12:16:32Z

Coverage increased (+0.01%) to 31.855% when pulling 633ac28 on Jaskey:ROCKETMQ-172-log-improvement into a8fa05e on apache:develop.

coveralls · 2017-04-11T12:16:32Z

Coverage increased (+0.01%) to 31.855% when pulling 633ac28 on Jaskey:ROCKETMQ-172-log-improvement into a8fa05e on apache:develop.

vesense · 2017-04-13T06:21:11Z

LGTM

vongosling · 2017-04-13T05:59:22Z

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()));


Please replace {} with +

But this is not a sl4j 's logging, it's a exception, so direct string is needed.

coveralls · 2017-04-14T08:52:17Z

Coverage increased (+2.8%) to 34.664% when pulling 7909245 on Jaskey:ROCKETMQ-172-log-improvement into a8fa05e on apache:develop.

coveralls · 2017-04-14T08:52:17Z

Coverage increased (+2.8%) to 34.664% when pulling 7909245 on Jaskey:ROCKETMQ-172-log-improvement into a8fa05e on apache:develop.

coveralls · 2017-04-14T08:52:17Z

Coverage increased (+2.8%) to 34.664% when pulling 7909245 on Jaskey:ROCKETMQ-172-log-improvement into a8fa05e on apache:develop.

coveralls · 2017-04-14T09:37:33Z

Coverage increased (+2.8%) to 34.604% when pulling c46e4c6 on Jaskey:ROCKETMQ-172-log-improvement into a8fa05e on apache:develop.

shroman · 2017-04-17T02:47:28Z

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()));


How about writing it this way?
new MQClientException("send request [" + request+ "] failed to " + addr, responseFuture.getCause())

Or ".request: " -> ". Request: "

I would modify the typo , your NO2. advice

shroman · 2017-04-17T02:48:17Z

client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java

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


" request: " -> ", request: "

shroman · 2017-04-17T02:50:52Z

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


NEW, OLD look ugly to me :)
How about

log.info("name server address updated from [{}] to [{}]", addrs, old);

?

@shroman , but OLD , NEW has been already exists in rocketmq's log. please see ConsumerGroupInfo.java for one sample.

just a suggestion to make it prettier ;)

Perhaps we can post another PR for some logging format, currently I would just retain the current way

coveralls · 2017-04-17T03:36:30Z

Coverage increased (+2.7%) to 34.586% when pulling ec7c8cd on Jaskey:ROCKETMQ-172-log-improvement into a8fa05e on apache:develop.

coveralls · 2017-04-17T03:36:30Z

Coverage increased (+2.7%) to 34.586% when pulling ec7c8cd on Jaskey:ROCKETMQ-172-log-improvement into a8fa05e on apache:develop.

coveralls · 2017-04-17T03:36:30Z

Coverage increased (+2.7%) to 34.586% when pulling ec7c8cd on Jaskey:ROCKETMQ-172-log-improvement into a8fa05e on apache:develop.

coveralls · 2017-04-17T08:35:23Z

Coverage increased (+2.7%) to 34.492% when pulling 1e8b575 on Jaskey:ROCKETMQ-172-log-improvement into a8fa05e on apache:develop.

coveralls · 2017-04-17T08:35:23Z

Coverage increased (+2.7%) to 34.492% when pulling 1e8b575 on Jaskey:ROCKETMQ-172-log-improvement into a8fa05e on apache:develop.

coveralls · 2017-04-17T08:35:23Z

Coverage increased (+2.7%) to 34.492% when pulling 1e8b575 on Jaskey:ROCKETMQ-172-log-improvement into a8fa05e on apache:develop.

Jaskey · 2017-04-17T08:37:16Z

@shroman Please review again

shroman

+1

vongosling · 2017-04-17T11:19:49Z

+1

dongeforever · 2017-04-17T11:24:53Z

LGTM +1

dongeforever · 2017-04-17T11:32:50Z

@Jaskey This PR has been merged

Jaskey · 2017-04-17T12:27:29Z

@dongeforever @vongosling @shroman

Thank you guys!

[ISSUE #22] codeCov for mqtt.cs.session

improve error message when pull exception

633ac28

vongosling reviewed Apr 14, 2017

View reviewed changes

log when nameserver address list is updated

7909245

log when chosen nameserver address is updated

c46e4c6

shroman reviewed Apr 17, 2017

View reviewed changes

modify typo

ec7c8cd

log topic when topic route info table is updated

1e8b575

shroman approved these changes Apr 17, 2017

View reviewed changes

asfgit pushed a commit that referenced this pull request Apr 17, 2017

[ROCKETMQ-172]log improvement for rocketmq client closes #90

c183e0d

Jaskey closed this Apr 17, 2017

Jaskey deleted the ROCKETMQ-172-log-improvement branch April 17, 2017 12:27

asfgit pushed a commit that referenced this pull request Jun 6, 2017

[ROCKETMQ-172]log improvement for rocketmq client closes #90

15c2b55

JiaMingLiu93 pushed a commit to JiaMingLiu93/rocketmq that referenced this pull request May 28, 2020

[ROCKETMQ-172]log improvement for rocketmq client closes apache#90

808fc46

JiaMingLiu93 pushed a commit to JiaMingLiu93/rocketmq that referenced this pull request May 28, 2020

[ROCKETMQ-172]log improvement for rocketmq client closes apache#90

179ac69

pingww pushed a commit that referenced this pull request Aug 26, 2022

Merge pull request #90 from YxAc/improve_mqtt_cs_session_codeCov

0c20a2e

[ISSUE #22] codeCov for mqtt.cs.session

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ROCKETMQ-172]log improvement for rocketmq client #90

[ROCKETMQ-172]log improvement for rocketmq client #90

Jaskey commented Apr 11, 2017 •

edited

Loading

coveralls commented Apr 11, 2017

coveralls commented Apr 11, 2017

coveralls commented Apr 11, 2017

vesense commented Apr 13, 2017

vongosling Apr 13, 2017

Jaskey Apr 14, 2017

coveralls commented Apr 14, 2017

coveralls commented Apr 14, 2017

coveralls commented Apr 14, 2017

coveralls commented Apr 14, 2017 •

edited

Loading

shroman Apr 17, 2017

shroman Apr 17, 2017

Jaskey Apr 17, 2017

shroman Apr 17, 2017

shroman Apr 17, 2017

Jaskey Apr 17, 2017

shroman Apr 17, 2017

Jaskey Apr 17, 2017

coveralls commented Apr 17, 2017

coveralls commented Apr 17, 2017

coveralls commented Apr 17, 2017

coveralls commented Apr 17, 2017

coveralls commented Apr 17, 2017

coveralls commented Apr 17, 2017

Jaskey commented Apr 17, 2017

shroman left a comment

vongosling commented Apr 17, 2017

dongeforever commented Apr 17, 2017

dongeforever commented Apr 17, 2017

Jaskey commented Apr 17, 2017

[ROCKETMQ-172]log improvement for rocketmq client #90

[ROCKETMQ-172]log improvement for rocketmq client #90

Conversation

Jaskey commented Apr 11, 2017 • edited Loading

coveralls commented Apr 11, 2017

coveralls commented Apr 11, 2017

coveralls commented Apr 11, 2017

vesense commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 14, 2017

coveralls commented Apr 14, 2017

coveralls commented Apr 14, 2017

coveralls commented Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 17, 2017

coveralls commented Apr 17, 2017

coveralls commented Apr 17, 2017

coveralls commented Apr 17, 2017

coveralls commented Apr 17, 2017

coveralls commented Apr 17, 2017

Jaskey commented Apr 17, 2017

shroman left a comment

Choose a reason for hiding this comment

vongosling commented Apr 17, 2017

dongeforever commented Apr 17, 2017

dongeforever commented Apr 17, 2017

Jaskey commented Apr 17, 2017

Jaskey commented Apr 11, 2017 •

edited

Loading

coveralls commented Apr 14, 2017 •

edited

Loading