You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by lindzh <gi...@git.apache.org> on 2017/06/15 07:43:33 UTC
[GitHub] incubator-rocketmq pull request #120: [ROCKETMQ-224] add rocketmq client log...
GitHub user lindzh opened a pull request:
https://github.com/apache/incubator-rocketmq/pull/120
[ROCKETMQ-224] add rocketmq client log4j2 logging support
When using RocketMQ client,we can only using logback or log4j for logging. If we using log4j2,there is no client log.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lindzh/incubator-rocketmq fix_client_logger
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/120.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 #120
----
commit 4b4666a5b3279d66bbc1406ec9537b3383c0cdcc
Author: 鲁般 <de...@alibaba-inc.com>
Date: 2017-06-15T07:31:52Z
add rocketmq client log4j2 logging support
----
---
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 #120: [ROCKETMQ-224] add rocketmq client log...
Posted by vsair <gi...@git.apache.org>.
Github user vsair commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/120#discussion_r130556323
--- Diff: client/src/main/java/org/apache/rocketmq/client/log/ClientLogger.java ---
@@ -99,7 +111,12 @@ private static Logger createLogger(final String loggerName) {
}
public static Logger getLog() {
- return log;
+ if (log == null) {
--- End diff --
why not modify the init method?
---
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 #120: [ROCKETMQ-224] add rocketmq client log...
Posted by lindzh <gi...@git.apache.org>.
Github user lindzh commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/120#discussion_r123936939
--- Diff: client/pom.xml ---
@@ -37,13 +37,31 @@
<groupId>${project.groupId}</groupId>
<artifactId>rocketmq-common</artifactId>
</dependency>
+
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
+
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
</dependency>
+
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-core</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-api</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-slf4j-impl</artifactId>
--- End diff --
As log4j is widely used in example,add this test in example will make it confused to choose log moudle.
---
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 #120: [ROCKETMQ-224] add rocketmq client log...
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/120#discussion_r130550009
--- Diff: client/src/test/java/org/apache/rocketmq/client/log/ClientLogTest.java ---
@@ -32,15 +33,19 @@
LOG_DIR = System.getProperty(CLIENT_LOG_ROOT, "${user.home}/logs/rocketmqlogs");
}
+ // FIXME: Workarond for concret implementation for slf4j, is there any better solution for all slf4j implementations in one class ? 2017/8/1
--- End diff --
we'd better remember this task and fix it later.
---
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 #120: [ROCKETMQ-224] add rocketmq client log4j2 log...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/120
[](https://:/builds/11981660)
Coverage increased (+0.7%) to 39.271% when pulling **4b4666a5b3279d66bbc1406ec9537b3383c0cdcc on lindzh:fix_client_logger** into **0c5e53db6f4d0ed9f25747379a8b679e2da5392d on apache:master**.
---
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 #120: [ROCKETMQ-224] add rocketmq client log...
Posted by lindzh <gi...@git.apache.org>.
GitHub user lindzh reopened a pull request:
https://github.com/apache/incubator-rocketmq/pull/120
[ROCKETMQ-224] add rocketmq client log4j2 logging support
When using RocketMQ client,we can only using logback or log4j for logging. If we using log4j2,there is no client log.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lindzh/incubator-rocketmq fix_client_logger
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/120.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 #120
----
commit 4b4666a5b3279d66bbc1406ec9537b3383c0cdcc
Author: 鲁般 <de...@alibaba-inc.com>
Date: 2017-06-15T07:31:52Z
add rocketmq client log4j2 logging support
----
---
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 #120: [ROCKETMQ-224] add rocketmq client log...
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/120#discussion_r130515522
--- Diff: client/src/main/java/org/apache/rocketmq/client/log/ClientLogger.java ---
@@ -27,16 +28,19 @@
public static final String CLIENT_LOG_ROOT = "rocketmq.client.logRoot";
public static final String CLIENT_LOG_MAXINDEX = "rocketmq.client.logFileMaxIndex";
public static final String CLIENT_LOG_LEVEL = "rocketmq.client.logLevel";
+
private static Logger log;
- static {
- log = createLogger(LoggerName.CLIENT_LOGGER_NAME);
+ public static Class logClass = null;
--- End diff --
Add logClass and its getter for test purpose only?
---
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 #120: [ROCKETMQ-224] add rocketmq client log...
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/120#discussion_r124163372
--- Diff: client/src/test/java/org/apache/rocketmq/client/log/ClientLogTest.java ---
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.client.log;
+
+import org.junit.Test;
+
+import java.util.Date;
+
+public class ClientLogTest {
+
+ @Test
+ public void testLog4j2() {
+ ClientLogger.getLog().info("logg4j test " + new Date());
--- End diff --
Any assert or thrown exception?
---
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 #120: [ROCKETMQ-224] add rocketmq client log...
Posted by lizhanhui <gi...@git.apache.org>.
Github user lizhanhui commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/120#discussion_r130515214
--- Diff: client/src/test/java/org/apache/rocketmq/client/log/ClientLogTest.java ---
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.client.log;
+
+import junit.framework.Assert;
--- End diff --
We'd better use org.junit.Assert.
---
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 #120: [ROCKETMQ-224] add rocketmq client log...
Posted by lindzh <gi...@git.apache.org>.
Github user lindzh commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/120#discussion_r124972804
--- Diff: client/src/test/java/org/apache/rocketmq/client/log/ClientLogTest.java ---
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.rocketmq.client.log;
+
+import org.junit.Test;
+
+import java.util.Date;
+
+public class ClientLogTest {
+
+ @Test
+ public void testLog4j2() {
+ ClientLogger.getLog().info("logg4j test " + new Date());
--- End diff --
Assert has been added.
---
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 #120: [ROCKETMQ-224] add rocketmq client log...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-rocketmq/pull/120
---
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 #120: [ROCKETMQ-224] add rocketmq client log4j2 log...
Posted by vongosling <gi...@git.apache.org>.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/120
Now, LGTM. this pr also fix the rocketmq's logger appender bug when using any concrete implementation, no matter log4j, log4j2 or logback.
---
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 #120: [ROCKETMQ-224] add rocketmq client log...
Posted by lindzh <gi...@git.apache.org>.
Github user lindzh commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/120#discussion_r130544050
--- Diff: client/src/main/java/org/apache/rocketmq/client/log/ClientLogger.java ---
@@ -27,16 +28,19 @@
public static final String CLIENT_LOG_ROOT = "rocketmq.client.logRoot";
public static final String CLIENT_LOG_MAXINDEX = "rocketmq.client.logFileMaxIndex";
public static final String CLIENT_LOG_LEVEL = "rocketmq.client.logLevel";
+
private static Logger log;
- static {
- log = createLogger(LoggerName.CLIENT_LOGGER_NAME);
+ public static Class logClass = null;
--- End diff --
Fixed it with reflect
---
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 #120: [ROCKETMQ-224] add rocketmq client log4j2 log...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/120
[](https://:/builds/11981619)
Coverage decreased (-0.1%) to 38.464% when pulling **4b4666a5b3279d66bbc1406ec9537b3383c0cdcc on lindzh:fix_client_logger** into **0c5e53db6f4d0ed9f25747379a8b679e2da5392d on apache:master**.
---
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 #120: [ROCKETMQ-224] add rocketmq client log...
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/120#discussion_r123714561
--- Diff: pom.xml ---
@@ -638,6 +638,16 @@
<artifactId>log4j-core</artifactId>
<version>2.7</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
--- End diff --
Why could i depend log4j2 core directly in dependency management?
---
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 #120: [ROCKETMQ-224] add rocketmq client log...
Posted by lindzh <gi...@git.apache.org>.
Github user lindzh closed the pull request at:
https://github.com/apache/incubator-rocketmq/pull/120
---
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 #120: [ROCKETMQ-224] add rocketmq client log4j2 log...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/120
[](https://coveralls.io/builds/12635815)
Coverage increased (+0.2%) to 38.771% when pulling **0ea6994712784d700aa365c2e121306b898b3355 on lindzh:fix_client_logger** into **0c5e53db6f4d0ed9f25747379a8b679e2da5392d on apache:master**.
---
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 #120: [ROCKETMQ-224] add rocketmq client log4j2 log...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/120
[](https://coveralls.io/builds/12201519)
Coverage increased (+0.6%) to 39.189% when pulling **6ab4ad4553b70bfc39f7cacaf178ba9f01fd69f4 on lindzh:fix_client_logger** into **0c5e53db6f4d0ed9f25747379a8b679e2da5392d on apache:master**.
---
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 #120: [ROCKETMQ-224] add rocketmq client log...
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/120#discussion_r123713735
--- Diff: client/pom.xml ---
@@ -37,13 +37,31 @@
<groupId>${project.groupId}</groupId>
<artifactId>rocketmq-common</artifactId>
</dependency>
+
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
+
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
</dependency>
+
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-core</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-api</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-slf4j-impl</artifactId>
--- End diff --
Could we put log4j2 test in rocketmq example module ?
---
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 #120: [ROCKETMQ-224] add rocketmq client log...
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/120#discussion_r124163160
--- Diff: client/src/main/java/org/apache/rocketmq/client/log/ClientLogger.java ---
@@ -35,16 +36,19 @@
private static Logger createLogger(final String loggerName) {
String logConfigFilePath =
- System.getProperty("rocketmq.client.log.configFile",
- System.getenv("ROCKETMQ_CLIENT_LOG_CONFIGFILE"));
+ System.getProperty("rocketmq.client.log.configFile",
+ System.getenv("ROCKETMQ_CLIENT_LOG_CONFIGFILE"));
--- End diff --
Did you use the right code style? 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 #120: [ROCKETMQ-224] add rocketmq client log4j2 log...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/120
[](https://coveralls.io/builds/12635815)
Coverage increased (+0.2%) to 38.771% when pulling **0ea6994712784d700aa365c2e121306b898b3355 on lindzh:fix_client_logger** into **0c5e53db6f4d0ed9f25747379a8b679e2da5392d on apache:master**.
---
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 #120: [ROCKETMQ-224] add rocketmq client log4j2 log...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/120
[](https://coveralls.io/builds/12590707)
Coverage increased (+0.6%) to 39.21% when pulling **04e5a1e030c82e4c9175ed544a8d059eecbd6a72 on lindzh:fix_client_logger** into **0c5e53db6f4d0ed9f25747379a8b679e2da5392d on apache:master**.
---
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.
---