ctubbsii commented on a change in pull request #146:
URL: https://github.com/apache/accumulo-testing/pull/146#discussion_r681404309
##########
File path: pom.xml
##########
@@ -94,15 +106,51 @@
<artifactId>hadoop-client-runtime</artifactId>
<version>${hadoop.version}</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-api</artifactId>
+ <version>${log4j2.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-core</artifactId>
+ <version>${log4j2.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-jcl</artifactId>
+ <version>${log4j2.version}</version>
+ </dependency>
+ <!--
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-log4j12</artifactId>
+ <version>${slf4j.version}</version>
+ </dependency>
+ -->
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-slf4j-impl</artifactId>
+ <version>${log4j2.version}</version>
+ </dependency>
+ <!--
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-1.2-api</artifactId>
+ <version>${log4j2.version}</version>
+ </dependency>
+ -->
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
<version>${slf4j.version}</version>
</dependency>
<dependency>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
- <version>${slf4j.version}</version>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-bom</artifactId>
+ <version>2.14.1</version>
+ <type>pom</type>
+ <scope>runtime</scope>
Review comment:
BOM files should be used with the import scope, and declared in the
`<dependencyManagement />` section. The version here could use the
`${log4j2.version}` property instead of specifying it explicitly here.
##########
File path: pom.xml
##########
@@ -48,6 +50,11 @@
</dependencies>
</dependencyManagement>
<dependencies>
+ <dependency>
+ <groupId>com.github.edwgiz</groupId>
+ <artifactId>maven-shade-plugin.log4j2-cachefile-transformer</artifactId>
+ <version>2.1</version>
+ </dependency>
Review comment:
This should not be specified as a dependency of this project, as it is
only a dependency of the shade plugin, in order to add the transformer to that.
##########
File path: pom.xml
##########
@@ -29,12 +29,14 @@
<description>Testing tools for Apache Accumulo</description>
<properties>
<accumulo.version>2.1.0-SNAPSHOT</accumulo.version>
+ <disruptor.version>3.4.4</disruptor.version>
<eclipseFormatterStyle>${project.basedir}/contrib/Eclipse-Accumulo-Codestyle.xml</eclipseFormatterStyle>
<hadoop.version>3.2.0</hadoop.version>
+ <log4j2.version>2.14.1</log4j2.version>
Review comment:
Not sure if it's useful to have a property for this, if you use the BOM,
since the BOM should eliminate the need to use this property more than one
time. However, we may still want to be able to set the value with
`-Dlog4j2.version=` when building to override this, in which case, the property
would still need to be here, even if it's only ever used once (for the BOM).
##########
File path: pom.xml
##########
@@ -94,15 +106,51 @@
<artifactId>hadoop-client-runtime</artifactId>
<version>${hadoop.version}</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-api</artifactId>
+ <version>${log4j2.version}</version>
+ </dependency>
Review comment:
Specifying the BOM in the dependencyManagement section eliminates the
need to specify any versions for any of these log4j2 jars in this section.
##########
File path: pom.xml
##########
@@ -374,6 +439,8 @@
<transformers>
<!-- Hadoop uses service loader to find filesystem impls,
without this may not find them -->
<transformer
implementation="org.apache.maven.plugins.shade.resource.ServicesResourceTransformer"
/>
+ <!-- New transformer. Stops "unrecognized format
specifier" error. -->
Review comment:
Let's add a comment with a link to
`https://github.com/edwgiz/maven-shaded-log4j-transformer` in case anybody has
questions about what this does. The README there should answer any additional
questions.
##########
File path: conf/log4j2.properties
##########
@@ -0,0 +1,57 @@
+#
+# 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.
+#
+
+## Log4j2 file that configures logging for processes other than Accumulo
services
+status = info
+dest = err
+name = AccumuloTestingDefaultLoggingProperties
+
+appender.console.type = Console
+appender.console.name = STDERR
+appender.console.target = SYSTEM_ERR
+appender.console.layout.type = PatternLayout
+# appender.console.layout.pattern = %style{%d{ISO8601}}{dim,cyan}
%style{[}{red}%style{%-8c{2}}{dim,blue}%style{]}{red}
%highlight{%-5p}%style{:}{red} %m%n
+appender.console.layout.pattern = %d{ISO8601} [%c{3}] %-5p: %m%n
+
+logger.accumulo.name = org.apache.accumulo
+logger.accumulo.level = WARN
+logger.accumulo.testing = DEBUG
Review comment:
This isn't quite right. This should be:
```suggestion
logger.accumulotesting.name = org.apache.accumulo.testing
logger.accumulotesting.level = DEBUG
```
The segment after `logger.` is a way of creating a named reference to a
logger. Once you have a reference, you need to tell log4j2 what logger name
pattern it refers to with `.name = ` and what minimum level to accept for that
logger reference with `.level = `.
So, in general:
```ini
logger.refA.name = some.prefix.pattern.here
logger.refA.level = DEBUG
logger.refB.name = some.other.prefix.here
logger.refB.level = WARN
```
In log4j1, you would have put the pattern in the logger reference itself,
but you no longer do that in log4j2, since the logger reference is created
explicitly with `logger.refName.name = pattern`. So, `logger.accumulo.testing =
DEBUG` doesn't do anything, since `.testing` isn't valid.
This error is repeated a few times below. Hopefully this explanation helps.
##########
File path: pom.xml
##########
@@ -94,15 +106,51 @@
<artifactId>hadoop-client-runtime</artifactId>
<version>${hadoop.version}</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-api</artifactId>
+ <version>${log4j2.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-core</artifactId>
+ <version>${log4j2.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-jcl</artifactId>
+ <version>${log4j2.version}</version>
+ </dependency>
+ <!--
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-log4j12</artifactId>
+ <version>${slf4j.version}</version>
+ </dependency>
+ -->
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-slf4j-impl</artifactId>
+ <version>${log4j2.version}</version>
+ </dependency>
+ <!--
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-1.2-api</artifactId>
+ <version>${log4j2.version}</version>
+ </dependency>
+ -->
Review comment:
Please remove any unused commented out entries.
##########
File path: conf/log4j2.properties
##########
@@ -0,0 +1,57 @@
+#
+# 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.
+#
+
+## Log4j2 file that configures logging for processes other than Accumulo
services
+status = info
+dest = err
+name = AccumuloTestingDefaultLoggingProperties
+
+appender.console.type = Console
+appender.console.name = STDERR
+appender.console.target = SYSTEM_ERR
+appender.console.layout.type = PatternLayout
+# appender.console.layout.pattern = %style{%d{ISO8601}}{dim,cyan}
%style{[}{red}%style{%-8c{2}}{dim,blue}%style{]}{red}
%highlight{%-5p}%style{:}{red} %m%n
+appender.console.layout.pattern = %d{ISO8601} [%c{3}] %-5p: %m%n
+
+logger.accumulo.name = org.apache.accumulo
+logger.accumulo.level = WARN
+logger.accumulo.testing = DEBUG
+# logger.org.apache.accumulo=WARN
+#logger.org.apache.accumulo.testing=DEBUG
Review comment:
Should remove any of these commented out lines that aren't used.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
