You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by shroman <gi...@git.apache.org> on 2016/12/22 08:30:04 UTC
[GitHub] incubator-rocketmq pull request #3: Logging instead of printStack(). Fix for...
GitHub user shroman opened a pull request:
https://github.com/apache/incubator-rocketmq/pull/3
Logging instead of printStack(). Fix for ROCKETMQ-6.
See https://issues.apache.org/jira/browse/ROCKETMQ-6
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shroman/incubator-rocketmq ROCKETMQ-6
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/3.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 #3
----
commit 7d986dadaf648641fd04fbd6be25d7705bf0973a
Author: shtykh_roman <rs...@yahoo.com>
Date: 2016-12-22T08:27:51Z
Logging instead of printStack(). Fix for ROCKETMQ-6.
----
---
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 #3: [ROCKETMQ-6] Use logger for exceptions i...
Posted by shroman <gi...@git.apache.org>.
Github user shroman closed the pull request at:
https://github.com/apache/incubator-rocketmq/pull/3
---
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 #3: [ROCKETMQ-6] Use logger for exceptions instead ...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/3
IMO, we should prefer slf4j or String.format effective format way to log in 4.x :-)
---
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 #3: [ROCKETMQ-6] Use logger for exceptions instead ...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/3
@lollipopjin I resolved the conflicts, now you can review.
How about squashing all commits when you merge after reviewing this issue? My concern is -- I merged this to the master, so if I squash, it will be very hard for you to review.
---
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 #3: [ROCKETMQ-6] Use logger for exceptions i...
Posted by wu-sheng <gi...@git.apache.org>.
Github user wu-sheng commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/3#discussion_r95562388
--- Diff: broker/src/main/java/org/apache/rocketmq/broker/processor/PullMessageProcessor.java ---
@@ -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()) {
--- End diff --
`log.isDebugEnabled()` is not necessary, and do not consistent with other codes log-style.
---
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 #3: Logging instead of printStack(). Fix for ROCKET...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/3
@shroman Thanks your PR
Could you abide our PR process(modify your PR subject and mention your jira address in the description), please follow this PR. https://github.com/apache/incubator-rocketmq/pull/5
---
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 #3: [ROCKETMQ-6] Use logger for exceptions instead ...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/3
Hi @zhouxinyu
I am not sure if I understand your 1st point though, but if you are saying about printing to `stdout` or `stderr` so that users can find problems with brokers from the command line, for instance, quickly,
I think this can be configured via logger xml configuration, right?
As for the 2nd point, I used `+` just because I am not sure what is the preferred way for the project -- both `+` and `{}` through the project. Shall we make a rule for this in http://rocketmq.apache.org/docs/code-guidelines/?
---
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 #3: Logging instead of printStack(). Fix for ROCKET...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/3
@vongosling sure, but where can I find the description of the process?
Btw, modifying the subject, etc. can be done when you merge. I will follow the process thoroughly in my subsequent PRs.
---
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 #3: [ROCKETMQ-6] Use logger for exceptions i...
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/3#discussion_r95563857
--- Diff: broker/src/main/java/org/apache/rocketmq/broker/processor/PullMessageProcessor.java ---
@@ -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()) {
--- End diff --
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 ;)
---
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 #3: [ROCKETMQ-6] Use logger for exceptions instead ...
Posted by lollipopjin <gi...@git.apache.org>.
Github user lollipopjin commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/3
@shroman Could you please squash your commits and fix conflicts.
---
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 #3: [ROCKETMQ-6] Use logger for exceptions instead ...
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/3
Hi, @shroman
About `e.printStackTrace()`, the error message should log to file surely, but can we keep the `e.printStackTrace()` if the call stack is in broker start process. IMO, print the error message to `stderr` will be more visualized, the developers can quick locate the reason why broker start failed.
And I have another question, why this PR prefer to use `+` to concat string instead of using format string?
---
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 #3: [ROCKETMQ-6] Use logger for exceptions instead ...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/3
@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.
Anyway, I think it's better to move `stdout` logging to slf4j etc. as much as possible.
---
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 #3: [ROCKETMQ-6] Use logger for exceptions i...
Posted by wu-sheng <gi...@git.apache.org>.
Github user wu-sheng commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/3#discussion_r95574183
--- Diff: broker/src/main/java/org/apache/rocketmq/broker/processor/PullMessageProcessor.java ---
@@ -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()) {
--- End diff --
Agree. Next time, I can bring a new pull request after yours 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 #3: [ROCKETMQ-6] Use logger for exceptions instead ...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/3
any update? or close it :-)
---
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 #3: [ROCKETMQ-6] Use logger for exceptions i...
Posted by wu-sheng <gi...@git.apache.org>.
Github user wu-sheng commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/3#discussion_r95564265
--- Diff: broker/src/main/java/org/apache/rocketmq/broker/processor/PullMessageProcessor.java ---
@@ -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()) {
--- End diff --
No related, just think you can change
---
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 #3: [ROCKETMQ-6] Use logger for exceptions instead ...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/3
[](https://coveralls.io/builds/12257293)
Coverage decreased (-12.4%) to 26.315% when pulling **0baccb7e3e92aef89d1532351e2a5ff48d0c9c1e on shroman:ROCKETMQ-6** into **9ad9ad064f32470ae7e61ca3c400c680e8ab5ab4 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 #3: [ROCKETMQ-6] Use logger for exceptions i...
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/3#discussion_r93905067
--- Diff: rocketmq-remoting/src/main/java/com/alibaba/rocketmq/remoting/common/ServiceThread.java ---
@@ -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());
--- End diff --
the same
---
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 #3: [ROCKETMQ-6] Use logger for exceptions instead ...
Posted by dongeforever <gi...@git.apache.org>.
Github user dongeforever commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/3
@shroman please resolve the conflicts and merge this PR
---
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 #3: [ROCKETMQ-6] Use logger for exceptions i...
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/3#discussion_r93904989
--- Diff: rocketmq-common/src/main/java/com/alibaba/rocketmq/common/MixAll.java ---
@@ -287,8 +287,8 @@ public static void printObjectProperties(final Logger log, final Object object,
}
}
- if (log != null) {
- log.info(name + "=" + value);
+ if (logger != null) {
+ logger.info(name + "=" + value);
--- 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 pull request #3: [ROCKETMQ-6] Use logger for exceptions i...
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/3#discussion_r93905030
--- Diff: rocketmq-remoting/src/main/java/com/alibaba/rocketmq/remoting/common/ServiceThread.java ---
@@ -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 + " "
--- End diff --
the same question :-)
---
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 #3: [ROCKETMQ-6] Use logger for exceptions i...
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/3#discussion_r95571236
--- Diff: broker/src/main/java/org/apache/rocketmq/broker/processor/PullMessageProcessor.java ---
@@ -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()) {
--- End diff --
For this particular place, this can be removed, I agree. But it is good to concenrate the attention on what PR is about.
---
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 #3: [ROCKETMQ-6] Use logger for exceptions instead ...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/3
Ok, if it's good to merge, I will resolve the conflicts and merge.
---
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 #3: [ROCKETMQ-6] Use logger for exceptions instead ...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/3
Merged to develop branch.
---
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 #3: [ROCKETMQ-6] Use logger for exceptions instead ...
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/3
@shroman Please use parameterized template formatting way. See http://slf4j.org/faq.html section "What is the fastest way of (not) logging?"
---
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 #3: [ROCKETMQ-6] Use logger for exceptions instead ...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/3
@vongosling I completely agree with you
---
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 #3: [ROCKETMQ-6] Use logger for exceptions instead ...
Posted by shroman <gi...@git.apache.org>.
Github user shroman commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/3
@vongosling I modified as you requested ;) Please have a look.
---
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 #3: [ROCKETMQ-6] Use logger for exceptions i...
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/3#discussion_r93905062
--- Diff: rocketmq-remoting/src/main/java/com/alibaba/rocketmq/remoting/common/ServiceThread.java ---
@@ -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);
--- End diff --
the same
---
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.
---