-
Notifications
You must be signed in to change notification settings - Fork 11.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ROCKETMQ-6] Use logger for exceptions instead of e.printStackTrace(). #3
Conversation
@vongosling sure, but where can I find the description of the process? Btw, modifying the subject, etc. can be done when you merge. |
@vongosling I modified as you requested ;) Please have a look. |
4114084
to
297d903
Compare
JIRA Issue: https://issues.apache.org/jira/browse/ROCKETMQ-6 Merge branch 'master' into ROCKETMQ-6
@shroman Could you please squash your commits and fix conflicts. |
@lollipopjin I resolved the conflicts, now you can review. |
Hi, @shroman About And I have another question, why this PR prefer to use |
Hi @zhouxinyu I am not sure if I understand your 1st point though, but if you are saying about printing to As for the 2nd point, I used |
@shroman Please use parameterized template formatting way. See http://slf4j.org/faq.html section "What is the fastest way of (not) logging?" |
JIRA issue: https://issues.apache.org/jira/browse/ROCKETMQ-6 Changed logging to use parameterized template formatting.
@lizhanhui Ok. I changed my modifications to parameterized template formatting. @zhouxinyu If you feel this issue needs more careful logging consideration, I don't mind if you assign this JIRA issue to yourself ;) You are working with the project on the daily basis and can tune it better. |
IMO, we should prefer slf4j or String.format effective format way to log in 4.x :-) |
@vongosling I completely agree with you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one suggestion.
@@ -87,8 +87,8 @@ private RemotingCommand processRequest(final Channel channel, RemotingCommand re | |||
|
|||
response.setOpaque(request.getOpaque()); | |||
|
|||
if (LOG.isDebugEnabled()) { | |||
LOG.debug("receive PullMessage request command, {}", request); | |||
if (log.isDebugEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log.isDebugEnabled()
is not necessary, and do not consistent with other codes log-style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is it related to this PR? Please see the title.
That is from the original code, probably you can find ten more small improvements in files I edited but lines I haven't even touched ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No related, just think you can change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this particular place, this can be removed, I agree. But it is good to concenrate the attention on what PR is about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Next time, I can bring a new pull request after yours merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏🏻👏🏻💪
@shroman please resolve the conflicts and merge this PR |
if (log != null) { | ||
log.info(name + "=" + value); | ||
if (logger != null) { | ||
logger.info(name + "=" + value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please replace {} with + :-)
@@ -69,10 +69,10 @@ public void shutdown(final boolean interrupt) { | |||
long beginTime = System.currentTimeMillis(); | |||
this.thread.join(this.getJointime()); | |||
long eclipseTime = System.currentTimeMillis() - beginTime; | |||
STLOG.info("join thread " + this.getServiceName() + " eclipse time(ms) " + eclipseTime + " " | |||
log.info("join thread " + this.getServiceName() + " eclipse time(ms) " + eclipseTime + " " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same question :-)
@@ -86,7 +86,7 @@ public void stop() { | |||
|
|||
public void stop(final boolean interrupt) { | |||
this.stopped = true; | |||
STLOG.info("stop thread " + this.getServiceName() + " interrupt " + interrupt); | |||
log.info("stop thread " + this.getServiceName() + " interrupt " + interrupt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same
@@ -101,7 +101,7 @@ public void stop(final boolean interrupt) { | |||
|
|||
public void makeStop() { | |||
this.stopped = true; | |||
STLOG.info("makestop thread " + this.getServiceName()); | |||
log.info("makestop thread " + this.getServiceName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same
any update? or close it :-) |
Ok, if it's good to merge, I will resolve the conflicts and merge. |
Merged to develop branch. |
1 similar comment
# This is the 1st commit message: add acl event Signed-off-by: lyx <1419360299@qq.com> # This is the commit message apache#2: add update acl config event Signed-off-by: lyx <1419360299@qq.com> # This is the commit message apache#3: refactor(event): fix rename Signed-off-by: lyx <1419360299@qq.com> # This is the commit message apache#4: refactor(acl): track acl update event Signed-off-by: lyx <1419360299@qq.com> # Conflicts: # acl/src/main/java/org/apache/rocketmq/acl/plain/PlainPermissionManager.java
See https://issues.apache.org/jira/browse/ROCKETMQ-6