You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/02/21 09:06:27 UTC
[GitHub] [accumulo] ctubbsii opened a new pull request #1528: Switch to
Log4j2
ctubbsii opened a new pull request #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528
WIP - will update description later
----------------------------------------------------------------
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] [accumulo] milleruntime commented on a change in pull request
#1528: Switch to Log4j2
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528#discussion_r383940546
##########
File path: core/pom.xml
##########
@@ -145,13 +145,13 @@
<scope>test</scope>
</dependency>
<dependency>
- <groupId>org.easymock</groupId>
- <artifactId>easymock</artifactId>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-slf4j-impl</artifactId>
Review comment:
A log4j package with "impl" in the name makes me think this isn't public API. Is this for public use?
----------------------------------------------------------------
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] [accumulo] ctubbsii commented on a change in pull request #1528:
Switch to Log4j2
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528#discussion_r383990571
##########
File path: pom.xml
##########
@@ -235,6 +235,11 @@
<artifactId>protobuf-java</artifactId>
<version>3.7.1</version>
</dependency>
+ <dependency>
+ <groupId>com.lmax</groupId>
+ <artifactId>disruptor</artifactId>
+ <version>3.4.2</version>
+ </dependency>
Review comment:
So, this is used as an optional dependency of log4j2, that enables the high-performance asynchronous loggers. It is required on the class path if `-Dlog4j2.contextSelector=org.apache.logging.log4j.core.async.AsyncLoggerContextSelector` system property is set. I set it by default in our example `accumulo-env.sh`, because there's no good reason to not have it, since it's ALv2 licensed has no deps.
----------------------------------------------------------------
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] [accumulo] EdColeman commented on a change in pull request #1528:
Switch to Log4j2
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528#discussion_r384029393
##########
File path: pom.xml
##########
@@ -235,6 +235,11 @@
<artifactId>protobuf-java</artifactId>
<version>3.7.1</version>
</dependency>
+ <dependency>
+ <groupId>com.lmax</groupId>
+ <artifactId>disruptor</artifactId>
+ <version>3.4.2</version>
+ </dependency>
Review comment:
The LMAX disrupter is basically a ring buffer that is designed to minimize locking and from most reports significantly improves performance. We should use it if at all possible. Enabling it and keeping the dependency seems to make it easier on users.
----------------------------------------------------------------
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] [accumulo] ctubbsii commented on issue #1528: Switch to Log4j2
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528#issuecomment-589643440
@EdColeman I think you'll be happy to know that I tested the automatic reconfiguration by simple editing of the log4j2.properties while the shell was running, and it worked perfectly. It's a great alternative to, and much more powerful than, the 'debug on' feature of the shell that was crippled in 2.0.
----------------------------------------------------------------
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] [accumulo] milleruntime commented on a change in pull request
#1528: Switch to Log4j2
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528#discussion_r383942412
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -544,8 +544,6 @@
"Properties in this category affect the behavior of the monitor web server."),
MONITOR_PORT("monitor.port.client", "9995", PropertyType.PORT,
"The listening port for the monitor's http service"),
- MONITOR_LOG4J_PORT("monitor.port.log4j", "4560", PropertyType.PORT,
- "The listening port for the monitor's log4j logging collection."),
Review comment:
It looks like this property was only used in MiniAccumulo?
----------------------------------------------------------------
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] [accumulo] ctubbsii commented on a change in pull request #1528:
Switch to Log4j2
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528#discussion_r383960772
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -544,8 +544,6 @@
"Properties in this category affect the behavior of the monitor web server."),
MONITOR_PORT("monitor.port.client", "9995", PropertyType.PORT,
"The listening port for the monitor's http service"),
- MONITOR_LOG4J_PORT("monitor.port.log4j", "4560", PropertyType.PORT,
- "The listening port for the monitor's log4j logging collection."),
Review comment:
Yes, though users could have used it, too. It's a NOOP now. The config property does nothing.
----------------------------------------------------------------
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] [accumulo] ctubbsii commented on issue #1528: Switch to Log4j2
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528#issuecomment-590634717
More details on the ZooKeeper issue I mentioned above is handled in a separate PR (#1531) and the referenced ZOOKEEPER JIRA issue.
----------------------------------------------------------------
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] [accumulo] milleruntime commented on issue #1528: Switch to Log4j2
Posted by GitBox <gi...@apache.org>.
milleruntime commented on issue #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528#issuecomment-590451149
Any ideas how to verify that the properties files were converted over correctly? This is rather tricky to review since all the files have completely different format now.
----------------------------------------------------------------
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] [accumulo] ctubbsii merged pull request #1528: Switch to Log4j2
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528
----------------------------------------------------------------
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] [accumulo] milleruntime commented on a change in pull request
#1528: Switch to Log4j2
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528#discussion_r383943708
##########
File path: pom.xml
##########
@@ -235,6 +235,11 @@
<artifactId>protobuf-java</artifactId>
<version>3.7.1</version>
</dependency>
+ <dependency>
+ <groupId>com.lmax</groupId>
+ <artifactId>disruptor</artifactId>
+ <version>3.4.2</version>
+ </dependency>
Review comment:
This looks like a new dependency. Is this needed by log4j2? What uses this?
----------------------------------------------------------------
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] [accumulo] ctubbsii commented on a change in pull request #1528:
Switch to Log4j2
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528#discussion_r383960772
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -544,8 +544,6 @@
"Properties in this category affect the behavior of the monitor web server."),
MONITOR_PORT("monitor.port.client", "9995", PropertyType.PORT,
"The listening port for the monitor's http service"),
- MONITOR_LOG4J_PORT("monitor.port.log4j", "4560", PropertyType.PORT,
- "The listening port for the monitor's log4j logging collection."),
Review comment:
Users could have used it, but it's a NOOP now.
----------------------------------------------------------------
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] [accumulo] ctubbsii commented on a change in pull request #1528:
Switch to Log4j2
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528#discussion_r383959763
##########
File path: core/pom.xml
##########
@@ -145,13 +145,13 @@
<scope>test</scope>
</dependency>
<dependency>
- <groupId>org.easymock</groupId>
- <artifactId>easymock</artifactId>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-slf4j-impl</artifactId>
Review comment:
It is not public API. This is the "slf4j-api" implementation that bridges to the "log4j2-api" at runtime. It is needed on the runtime classpath.
----------------------------------------------------------------
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] [accumulo] EdColeman commented on a change in pull request #1528:
Switch to Log4j2
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528#discussion_r384029781
##########
File path: pom.xml
##########
@@ -235,6 +235,11 @@
<artifactId>protobuf-java</artifactId>
<version>3.7.1</version>
</dependency>
+ <dependency>
+ <groupId>com.lmax</groupId>
+ <artifactId>disruptor</artifactId>
+ <version>3.4.2</version>
+ </dependency>
Review comment:
The LMAX disrupter is basically a ring buffer that is designed to minimize locking and from most reports significantly improves performance. We should use it if at all possible. Enabling it and keeping the dependency seems to make it easier on users and should help to encourage its usage.
----------------------------------------------------------------
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] [accumulo] ctubbsii commented on issue #1528: Switch to Log4j2
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528#issuecomment-590554782
> Any ideas how to verify that the properties files were converted over correctly? This is rather tricky to review since all the files have completely different format now.
The config files need less review than the code. Unfortunately, since it's all related, I wasn't able to break it into separate pieces for easier review.
As for the config files:
None of the test config files (log4j2-test.properties) actually matter unless they affect a test. They are mostly personal preference while debugging build test failures. So, they don't really matter. But... I did verify the tests pass (after some initially failed before I converted them over), so those are all fine, for sure. They can be tweaked later if we want to log differently for our build testing. Log4j2 itself would also have logged something in the test output if the config files were malformed, and I didn't see anything like that. For the ITs in the test module, the ones that use minicluster are easily to verify also, by looking at the mini log directory.
The best way to test the config files in the tarball assembly is to use Uno (https://github.com/apache/fluo-uno) to start up an Accumulo instance.
Using Uno, it is easy to verify the logs get sent to the Monitor properly with the out-of-the-box configs, as well as to the log directories (log4j2-service.properties). Uno also makes it easy to run a shell and verify the console logging for the shell works well (log4j2.properties). (Try `debug` or `flush` to see various colorized output.)
----------------------------------------------------------------
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] [accumulo] ctubbsii commented on issue #1528: Switch to Log4j2
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528#issuecomment-589674542
Related ZooKeeper issues: https://issues.apache.org/jira/browse/ZOOKEEPER-1371 and https://issues.apache.org/jira/browse/ZOOKEEPER-850
The ClassNotFoundException I was seeing at runtime with ZK 3.4.14 was org.apache.log4j.jmx.HierarchyDynamicMBean ; this may be resolved in the latest 3.5.
----------------------------------------------------------------
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] [accumulo] ctubbsii edited a comment on issue #1528: Switch to
Log4j2
Posted by GitBox <gi...@apache.org>.
ctubbsii edited a comment on issue #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528#issuecomment-589945578
All ITs pass with these latest changes, except for:
```
org.apache.accumulo.test.fate.zookeeper.ZooLockTest.testTryLock
org.apache.accumulo.test.functional.GarbageCollectorIT.gcLotsOfCandidatesIT
org.apache.accumulo.test.replication.CyclicReplicationIT.dataIsNotOverReplicated
org.apache.accumulo.test.replication.KerberosReplicationIT.dataReplicatedToCorrectTable
org.apache.accumulo.test.replication.ReplicationIT.verifyReplicationTableConfig
org.apache.accumulo.test.replication.UnorderedWorkAssignerReplicationIT.dataWasReplicatedToThePeerWithoutDrain
```
which are all previously flakey tests.
----------------------------------------------------------------
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] [accumulo] ctubbsii commented on issue #1528: Switch to Log4j2
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528#issuecomment-589945578
All ITs pass with these latest changes, except for:
```
org.apache.accumulo.test.functional.GarbageCollectorIT.gcLotsOfCandidatesIT
org.apache.accumulo.test.replication.CyclicReplicationIT.dataIsNotOverReplicated
org.apache.accumulo.test.replication.UnorderedWorkAssignerReplicationIT.dataWasReplicatedToThePeer
```
which are all previously flakey tests.
----------------------------------------------------------------
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] [accumulo] ctubbsii commented on issue #1528: Switch to Log4j2
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1528: Switch to Log4j2
URL: https://github.com/apache/accumulo/pull/1528#issuecomment-589671711
Biggest frustration is that ZooKeeper (at least, 3.4; I haven't checked 3.5) has a hard runtime dependency on the log4j 1.2 jar, which could conflict with the log4j-1.2-api shim for log4j2 (which contains most log4j 1.2 API classes, but not all of those, including some that ZK needs). This doesn't matter so much or most of our code, since the ZK client code doesn't seem to need it, so we can just leave it off the class path for most processes. However, the ZK server seems to need it for some JMX stuff it is doing, which means that Minicluster, and anything that uses Minicluster will be affected (such as our ITs). Hopefully this is a non-issue in ZooKeeper 3.5, but I haven't checked.
----------------------------------------------------------------
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