You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/02/25 20:46:47 UTC
[GitHub] [zookeeper] ctubbsii opened a new pull request #1270:
ZOOKEEPER-3737: Detect log4j 1.2 jmx support better
ctubbsii opened a new pull request #1270: ZOOKEEPER-3737: Detect log4j 1.2 jmx support better
URL: https://github.com/apache/zookeeper/pull/1270
* Look for jmx class that exists only in the log4j 1.2 jar, but not in
the log4j2 1.2 compatibility jar.
* Check if disabled before attempting to detect log4j 1.2 jmx classes.
* Update log messages to highlight that they are referring to log4j 1.2
and not log4j2 or other versions.
* Minor javadoc fixup
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1270:
ZOOKEEPER-3737: Detect log4j 1.2 jmx support better
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1270: ZOOKEEPER-3737: Detect log4j 1.2 jmx support better
URL: https://github.com/apache/zookeeper/pull/1270#discussion_r384427776
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/jmx/ManagedUtil.java
##########
@@ -35,27 +35,25 @@
private static boolean isLog4jJmxEnabled() {
boolean enabled = false;
- try {
- Class.forName("org.apache.log4j.spi.LoggerRepository");
-
- if (Boolean.getBoolean("zookeeper.jmx.log4j.disable")) {
- LOG.info("Log4j found but jmx support is disabled.");
- } else {
+ if (Boolean.getBoolean("zookeeper.jmx.log4j.disable")) {
+ LOG.info("Log4j 1.2 jmx support is disabled by property.");
+ } else {
+ try {
+ Class.forName("org.apache.log4j.jmx.HierarchyDynamicMBean");
enabled = true;
- LOG.info("Log4j found with jmx enabled.");
+ LOG.info("Log4j 1.2 jmx support found and enabled.");
+ } catch (ClassNotFoundException e) {
+ LOG.info("Log4j 1.2 jmx support not found; jmx disabled.");
}
-
- } catch (ClassNotFoundException e) {
- LOG.info("Log4j not found.");
}
return enabled;
}
/**
- * Register the log4j JMX mbeans. Set environment variable
+ * Register the log4j JMX mbeans. Set system property
* "zookeeper.jmx.log4j.disable" to true to disable registration.
- * See http://logging.apache.org/log4j/1.2/apidocs/index.html?org/apache/log4j/jmx/package-summary.html
+ * @see http://logging.apache.org/log4j/1.2/apidocs/index.html?org/apache/log4j/jmx/package-summary.html
Review comment:
no problem from my side.
I was just asking
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] ctubbsii commented on issue #1270: ZOOKEEPER-3737:
Detect log4j 1.2 jmx support better
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1270: ZOOKEEPER-3737: Detect log4j 1.2 jmx support better
URL: https://github.com/apache/zookeeper/pull/1270#issuecomment-591061000
This doesn't remove the dependency on log4j 1.2 for this particular jmx support feature, but it does allow ZooKeeper to behave better when log4j 1.2 isn't on the class path.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1270:
ZOOKEEPER-3737: Detect log4j 1.2 jmx support better
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1270: ZOOKEEPER-3737: Detect log4j 1.2 jmx support better
URL: https://github.com/apache/zookeeper/pull/1270#discussion_r384126953
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/jmx/ManagedUtil.java
##########
@@ -35,27 +35,25 @@
private static boolean isLog4jJmxEnabled() {
boolean enabled = false;
- try {
- Class.forName("org.apache.log4j.spi.LoggerRepository");
-
- if (Boolean.getBoolean("zookeeper.jmx.log4j.disable")) {
- LOG.info("Log4j found but jmx support is disabled.");
- } else {
+ if (Boolean.getBoolean("zookeeper.jmx.log4j.disable")) {
+ LOG.info("Log4j 1.2 jmx support is disabled by property.");
+ } else {
+ try {
+ Class.forName("org.apache.log4j.jmx.HierarchyDynamicMBean");
enabled = true;
- LOG.info("Log4j found with jmx enabled.");
+ LOG.info("Log4j 1.2 jmx support found and enabled.");
+ } catch (ClassNotFoundException e) {
+ LOG.info("Log4j 1.2 jmx support not found; jmx disabled.");
}
-
- } catch (ClassNotFoundException e) {
- LOG.info("Log4j not found.");
}
return enabled;
}
/**
- * Register the log4j JMX mbeans. Set environment variable
+ * Register the log4j JMX mbeans. Set system property
* "zookeeper.jmx.log4j.disable" to true to disable registration.
- * See http://logging.apache.org/log4j/1.2/apidocs/index.html?org/apache/log4j/jmx/package-summary.html
+ * @see http://logging.apache.org/log4j/1.2/apidocs/index.html?org/apache/log4j/jmx/package-summary.html
Review comment:
Just for curiosity:
I am not sure it is a good thing to link to an external website using @see
It is not so relevant this link and javadoc tools may want to validate this link or to download?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1270:
ZOOKEEPER-3737: Detect log4j 1.2 jmx support better
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1270: ZOOKEEPER-3737: Detect log4j 1.2 jmx support better
URL: https://github.com/apache/zookeeper/pull/1270#discussion_r384153901
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/jmx/ManagedUtil.java
##########
@@ -35,27 +35,25 @@
private static boolean isLog4jJmxEnabled() {
boolean enabled = false;
- try {
- Class.forName("org.apache.log4j.spi.LoggerRepository");
-
- if (Boolean.getBoolean("zookeeper.jmx.log4j.disable")) {
- LOG.info("Log4j found but jmx support is disabled.");
- } else {
+ if (Boolean.getBoolean("zookeeper.jmx.log4j.disable")) {
+ LOG.info("Log4j 1.2 jmx support is disabled by property.");
+ } else {
+ try {
+ Class.forName("org.apache.log4j.jmx.HierarchyDynamicMBean");
enabled = true;
- LOG.info("Log4j found with jmx enabled.");
+ LOG.info("Log4j 1.2 jmx support found and enabled.");
+ } catch (ClassNotFoundException e) {
+ LOG.info("Log4j 1.2 jmx support not found; jmx disabled.");
}
-
- } catch (ClassNotFoundException e) {
- LOG.info("Log4j not found.");
}
return enabled;
}
/**
- * Register the log4j JMX mbeans. Set environment variable
+ * Register the log4j JMX mbeans. Set system property
* "zookeeper.jmx.log4j.disable" to true to disable registration.
- * See http://logging.apache.org/log4j/1.2/apidocs/index.html?org/apache/log4j/jmx/package-summary.html
+ * @see http://logging.apache.org/log4j/1.2/apidocs/index.html?org/apache/log4j/jmx/package-summary.html
Review comment:
I can change it back, though if you really want.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] asfgit closed pull request #1270: ZOOKEEPER-3737:
Detect log4j 1.2 jmx support better
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1270: ZOOKEEPER-3737: Detect log4j 1.2 jmx support better
URL: https://github.com/apache/zookeeper/pull/1270
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1270:
ZOOKEEPER-3737: Detect log4j 1.2 jmx support better
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1270: ZOOKEEPER-3737: Detect log4j 1.2 jmx support better
URL: https://github.com/apache/zookeeper/pull/1270#discussion_r384138614
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/jmx/ManagedUtil.java
##########
@@ -35,27 +35,25 @@
private static boolean isLog4jJmxEnabled() {
boolean enabled = false;
- try {
- Class.forName("org.apache.log4j.spi.LoggerRepository");
-
- if (Boolean.getBoolean("zookeeper.jmx.log4j.disable")) {
- LOG.info("Log4j found but jmx support is disabled.");
- } else {
+ if (Boolean.getBoolean("zookeeper.jmx.log4j.disable")) {
+ LOG.info("Log4j 1.2 jmx support is disabled by property.");
+ } else {
+ try {
+ Class.forName("org.apache.log4j.jmx.HierarchyDynamicMBean");
enabled = true;
- LOG.info("Log4j found with jmx enabled.");
+ LOG.info("Log4j 1.2 jmx support found and enabled.");
+ } catch (ClassNotFoundException e) {
+ LOG.info("Log4j 1.2 jmx support not found; jmx disabled.");
}
-
- } catch (ClassNotFoundException e) {
- LOG.info("Log4j not found.");
}
return enabled;
}
/**
- * Register the log4j JMX mbeans. Set environment variable
+ * Register the log4j JMX mbeans. Set system property
* "zookeeper.jmx.log4j.disable" to true to disable registration.
- * See http://logging.apache.org/log4j/1.2/apidocs/index.html?org/apache/log4j/jmx/package-summary.html
+ * @see http://logging.apache.org/log4j/1.2/apidocs/index.html?org/apache/log4j/jmx/package-summary.html
Review comment:
I'm not aware of any built-in link validation in javadoc. This just cleans it up slightly to use proper javadoc syntax to reference a resource.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] phunt commented on issue #1270: ZOOKEEPER-3737: Detect
log4j 1.2 jmx support better
Posted by GitBox <gi...@apache.org>.
phunt commented on issue #1270: ZOOKEEPER-3737: Detect log4j 1.2 jmx support better
URL: https://github.com/apache/zookeeper/pull/1270#issuecomment-593058005
+1, lgtm. I tested with 1.2 default as well as disable enabled (was not able to test v2 however)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services