[ROCKETMQ-172]log improvement for rocketmq client by Jaskey · Pull Request #90 · apache/rocketmq

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

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

                } 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

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


" request: " -> ", request: "

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


            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

Uh oh!

Conversation

Jaskey commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 11, 2017

Uh oh!

coveralls commented Apr 11, 2017

Uh oh!

coveralls commented Apr 11, 2017

Uh oh!

vesense commented Apr 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Apr 14, 2017

Uh oh!

coveralls commented Apr 14, 2017

Uh oh!

coveralls commented Apr 14, 2017

Uh oh!

coveralls commented Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Apr 17, 2017

Uh oh!

coveralls commented Apr 17, 2017

Uh oh!

coveralls commented Apr 17, 2017

Uh oh!

coveralls commented Apr 17, 2017

Uh oh!

coveralls commented Apr 17, 2017

Uh oh!

coveralls commented Apr 17, 2017

Uh oh!

Jaskey commented Apr 17, 2017

Uh oh!

shroman left a comment

Choose a reason for hiding this comment

Uh oh!

vongosling commented Apr 17, 2017

Uh oh!

dongeforever commented Apr 17, 2017

Uh oh!

dongeforever commented Apr 17, 2017

Uh oh!

Jaskey commented Apr 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Jaskey commented Apr 11, 2017 •

edited

Loading

coveralls commented Apr 14, 2017 •

edited

Loading