[ROCKETMQ-158]Remove logback dependency for rocketmq-tools #85

Jaskey · 2017-03-28T10:52:07Z

jira: https://issues.apache.org/jira/browse/ROCKETMQ-158

coveralls · 2017-03-28T12:05:30Z

Coverage increased (+0.03%) to 31.644% when pulling b5876bb on Jaskey:ROCKETMQ-158 into 203cb30 on apache:develop.

coveralls · 2017-03-28T12:05:30Z

Coverage increased (+0.03%) to 31.644% when pulling b5876bb on Jaskey:ROCKETMQ-158 into 203cb30 on apache:develop.

coveralls · 2017-03-28T12:16:27Z

Coverage increased (+0.04%) to 31.662% when pulling 53bb2a6 on Jaskey:ROCKETMQ-158 into 203cb30 on apache:develop.

coveralls · 2017-03-28T12:16:27Z

Coverage increased (+0.04%) to 31.662% when pulling 53bb2a6 on Jaskey:ROCKETMQ-158 into 203cb30 on apache:develop.

coveralls · 2017-03-28T12:16:27Z

Coverage increased (+0.04%) to 31.662% when pulling 53bb2a6 on Jaskey:ROCKETMQ-158 into 203cb30 on apache:develop.

coveralls · 2017-03-28T13:22:26Z

Coverage decreased (-0.04%) to 31.829% when pulling b7218ae on Jaskey:ROCKETMQ-158 into 7e37799 on apache:develop.

coveralls · 2017-03-28T13:22:26Z

Coverage decreased (-0.04%) to 31.829% when pulling b7218ae on Jaskey:ROCKETMQ-158 into 7e37799 on apache:develop.

coveralls · 2017-03-28T13:22:26Z

Coverage decreased (-0.04%) to 31.829% when pulling b7218ae on Jaskey:ROCKETMQ-158 into 7e37799 on apache:develop.

lizhanhui · 2017-03-28T15:45:53Z

The logic here shares much similar logic with ClientLogger class, can we extract the common part into a reusable method?

Jaskey · 2017-03-29T02:28:24Z

@lizhanhui

So do you agree this is indeed an issue that needed to be solved? If yes, I will try doing that.

But actually it is a little different from the clientLogger since clientlogger will use the resources file in the classpath by default while tools admin does not have config file in the classpath.

lizhanhui · 2017-03-29T02:42:08Z

So do you agree this is indeed an issue that needed to be solved? If yes, I will try doing that.

Changing specific dependency to a neutral logging framework is good to have as application developer may integrate tool module to their operation management system.

But actually it is a little different from the clientLogger since clientlogger will use the resources file in the classpath by default while tools admin does not have config file in the classpath.

I know this, but it does not look like a blocking issue.

coveralls · 2017-03-29T03:58:49Z

Coverage decreased (-0.08%) to 31.791% when pulling 3d3801b on Jaskey:ROCKETMQ-158 into 7e37799 on apache:develop.

Jaskey · 2017-03-29T06:42:04Z

@lizhanhui

please review the updated pr

lizhanhui · 2017-03-29T08:00:12Z

The conf folder has been moved to the distribution module, please rebase this PR on top of develop branch. I'll +1 thereafter.

vongosling · 2017-03-29T08:11:43Z

tools/src/main/java/org/apache/rocketmq/tools/command/MQAdminStartup.java

+                final String logfjConfigPath = rocketmqHome + "/conf/log4j_tools.xml";
+                LogUtils.configLog4j(logfjConfigPath,null);
            } else if (classType.getName().equals("ch.qos.logback.classic.LoggerContext")) {
-                Class<?> joranConfigurator;


Could we extract this log loader logic into one place

Yes, we may wrap here further @Jaskey

@vongosling the updated pr has been extract two logger config into util function. Do you mean the if else code snippet is need to wrap too?

Jaskey · 2017-03-29T13:07:56Z

@lizhanhui please review the updated pr, log4j_tools.xml has been move to distributions.

coveralls · 2017-03-29T13:29:27Z

Coverage increased (+0.008%) to 31.788% when pulling 980c1a3 on Jaskey:ROCKETMQ-158 into ab01386 on apache:develop.

coveralls · 2017-03-29T13:29:27Z

Coverage increased (+0.008%) to 31.788% when pulling 980c1a3 on Jaskey:ROCKETMQ-158 into ab01386 on apache:develop.

lizhanhui · 2017-04-12T14:36:23Z

common/src/main/java/org/apache/rocketmq/common/utils/LogUtils.java

+public class LogUtils {
+
+    /**
+     * config logback dymatically from provided file path, otherwise try to config from resource file


Typo here: dymatically --> dynamically

Thank you! I will update the commit soon.

lizhanhui · 2017-04-12T14:36:47Z

common/src/main/java/org/apache/rocketmq/common/utils/LogUtils.java

+    }
+
+    /**
+     * config log4j dymatically from provided file path, otherwise try to config from resource file. Notice that the log4j configuration file should be in xml format


dymatically same as above.

Fixed, thank you!

lizhanhui · 2017-04-12T14:37:48Z

+1

coveralls · 2017-04-14T03:43:06Z

Coverage increased (+2.8%) to 34.628% when pulling 4449b2b on Jaskey:ROCKETMQ-158 into ab01386 on apache:develop.

coveralls · 2017-04-14T08:42:38Z

Coverage increased (+2.8%) to 34.601% when pulling 6647aeb on Jaskey:ROCKETMQ-158 into ab01386 on apache:develop.

coveralls · 2017-04-14T08:42:38Z

Coverage increased (+2.8%) to 34.601% when pulling 6647aeb on Jaskey:ROCKETMQ-158 into ab01386 on apache:develop.

coveralls · 2017-04-14T08:42:38Z

Coverage increased (+2.8%) to 34.601% when pulling 6647aeb on Jaskey:ROCKETMQ-158 into ab01386 on apache:develop.

Jaskey · 2017-04-19T06:10:40Z

@zhouxinyu @shroman @vongosling
what's your advice guys?

vongosling · 2017-08-07T06:51:03Z

+1

zhouxinyu · 2017-12-01T12:06:46Z

Hi @Jaskey

Sorry to reply late, could you please resolve the conflicts, so we could merge this PR quickly.

vongosling · 2017-12-20T10:20:27Z

@Jaskey Could you pick up this pr

coveralls · 2018-01-05T05:23:51Z

Coverage decreased (-0.05%) to 39.908% when pulling 3327b9a on Jaskey:ROCKETMQ-158 into cc0b3d0 on apache:develop.

Jaskey · 2018-01-05T05:27:31Z

@zhouxinyu @vongosling , conflicts have been resolved, please review again. thank you

vongosling · 2018-07-14T02:38:28Z

@Jaskey I will close the pr, if you happened to the same question, please let me know.

vongosling reviewed Mar 29, 2017

View reviewed changes

lizhanhui reviewed Apr 12, 2017

View reviewed changes

zhouxinyu closed this Dec 1, 2017

zhouxinyu reopened this Dec 1, 2017

[ROCKETMQ-158]Remove logback dependency for rocketmq-tools

3327b9a

vongosling closed this Jul 14, 2018

[ROCKETMQ-158]Remove logback dependency for rocketmq-tools #85

[ROCKETMQ-158]Remove logback dependency for rocketmq-tools #85

Uh oh!

Conversation

Jaskey commented Mar 28, 2017

Uh oh!

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

Uh oh!

Uh oh!

coveralls commented Mar 28, 2017

Uh oh!

coveralls commented Mar 28, 2017

Uh oh!

coveralls commented Mar 28, 2017

Uh oh!

coveralls commented Mar 28, 2017

Uh oh!

coveralls commented Mar 28, 2017

Uh oh!

coveralls commented Mar 28, 2017

Uh oh!

coveralls commented Mar 28, 2017

Uh oh!

lizhanhui commented Mar 28, 2017

Uh oh!

Jaskey commented Mar 29, 2017

Uh oh!

lizhanhui commented Mar 29, 2017

Uh oh!

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

Uh oh!

Uh oh!

Jaskey commented Mar 29, 2017

Uh oh!

lizhanhui commented Mar 29, 2017

Uh oh!

vongosling Mar 29, 2017

Choose a reason for hiding this comment

Uh oh!

lizhanhui Mar 29, 2017

Choose a reason for hiding this comment

Uh oh!

Jaskey Mar 29, 2017

Choose a reason for hiding this comment

Uh oh!

Jaskey commented Mar 29, 2017

Uh oh!

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

Uh oh!

Uh oh!

coveralls commented Mar 29, 2017

Uh oh!

lizhanhui Apr 12, 2017

Choose a reason for hiding this comment

Uh oh!

Jaskey Apr 12, 2017

Choose a reason for hiding this comment

Uh oh!

lizhanhui Apr 12, 2017

Choose a reason for hiding this comment

Uh oh!

Jaskey Apr 14, 2017

Choose a reason for hiding this comment

Uh oh!

lizhanhui commented Apr 12, 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!

coveralls commented Apr 14, 2017

Uh oh!

coveralls commented Apr 14, 2017

Uh oh!

coveralls commented Apr 14, 2017

Uh oh!

Jaskey commented Apr 19, 2017

Uh oh!

vongosling commented Aug 7, 2017

Uh oh!

zhouxinyu commented Dec 1, 2017

coveralls commented Mar 28, 2017 •

edited

Loading

coveralls commented Mar 29, 2017 •

edited

Loading

coveralls commented Mar 29, 2017 •

edited

Loading

coveralls commented Apr 14, 2017 •

edited

Loading