[
https://issues.apache.org/jira/browse/KNOX-1462?focusedWorklogId=649258&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-649258
]
ASF GitHub Bot logged work on KNOX-1462:
----------------------------------------
Author: ASF GitHub Bot
Created on: 10/Sep/21 15:12
Start Date: 10/Sep/21 15:12
Worklog Time Spent: 10m
Work Description: risdenk commented on a change in pull request #488:
URL: https://github.com/apache/knox/pull/488#discussion_r706193865
##########
File path:
gateway-provider-ha/src/test/java/org/apache/knox/gateway/ha/provider/impl/AtlasZookeeperURLManagerTest.java
##########
@@ -47,6 +47,7 @@
@Before
public void setUp() throws Exception {
+ System.setProperty("zookeeper.jmx.log4j.disable", "true");
Review comment:
This property isn't cleared in tearDown?
This could affect other running tests in the jvm as well.
##########
File path:
gateway-server/src/test/java/org/apache/knox/gateway/topology/monitor/ZooKeeperConfigurationMonitorTest.java
##########
@@ -85,6 +85,8 @@ public static void setupSuite() throws Exception {
}
private static void configureAndStartZKCluster() throws Exception {
+ System.setProperty("zookeeper.jmx.log4j.disable", "true");
Review comment:
This property isn't cleared in tearDown?
This could affect other running tests in the jvm as well.
##########
File path: pom.xml
##########
@@ -1864,14 +1905,14 @@
</dependency>
<dependency>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-api</artifactId>
- <version>${slf4j.version}</version>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-api</artifactId>
+ <version>${log4j.version}</version>
</dependency>
<dependency>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
- <version>${slf4j.version}</version>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-core</artifactId>
+ <version>${log4j.version}</version>
Review comment:
Should probably use
https://mvnrepository.com/artifact/org.apache.logging.log4j/log4j-bom
##########
File path: gateway-demo-ldap/pom.xml
##########
@@ -74,18 +74,9 @@
</dependency>
<dependency>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-api</artifactId>
- </dependency>
-
- <dependency>
- <groupId>org.slf4j</groupId>
- <artifactId>slf4j-log4j12</artifactId>
- </dependency>
-
- <dependency>
- <groupId>log4j</groupId>
- <artifactId>log4j</artifactId>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-api</artifactId>
+ <version>${log4j.version}</version>
Review comment:
This shouldn't be here. `<version>` should only be in top level pom.
Ideally the top level pom should use the log4j2 bom -
https://mvnrepository.com/artifact/org.apache.logging.log4j/log4j-bom
##########
File path:
gateway-provider-ha/src/test/java/org/apache/knox/gateway/ha/provider/impl/HBaseZookeeperURLManagerTest.java
##########
@@ -49,6 +49,7 @@
@Before
public void setUp() throws Exception {
+ System.setProperty("zookeeper.jmx.log4j.disable", "true");
Review comment:
This property isn't cleared in tearDown?
This could affect other running tests in the jvm as well.
##########
File path:
gateway-provider-ha/src/test/java/org/apache/knox/gateway/ha/provider/impl/SOLRZookeeperURLManagerTest.java
##########
@@ -48,6 +48,7 @@
@Before
public void setUp() throws Exception {
+ System.setProperty("zookeeper.jmx.log4j.disable", "true");
Review comment:
This property isn't cleared in tearDown?
This could affect other running tests in the jvm as well.
##########
File path:
gateway-provider-ha/src/test/java/org/apache/knox/gateway/ha/provider/impl/HS2ZookeeperURLManagerTest.java
##########
@@ -43,6 +43,7 @@
@Before
public void setUp() throws Exception {
+ System.setProperty("zookeeper.jmx.log4j.disable", "true");
Review comment:
This property isn't cleared in tearDown?
This could affect other running tests in the jvm as well.
##########
File path:
gateway-server/src/test/java/org/apache/knox/gateway/services/security/impl/ZookeeperRemoteAliasServiceTest.java
##########
@@ -96,6 +96,8 @@ public static void setupSuite() throws Exception {
}
private static void configureAndStartZKCluster() throws Exception {
+ System.setProperty("zookeeper.jmx.log4j.disable", "true");
Review comment:
This property isn't cleared in tearDown?
This could affect other running tests in the jvm as well.
##########
File path:
gateway-i18n-logging-log4j/src/main/java/org/apache/knox/gateway/i18n/messages/loggers/log4j/Log4jMessageLogger.java
##########
@@ -37,42 +33,15 @@
@Override
public final boolean isLoggable( final MessageLevel level ) {
- return logger.isEnabledFor( toLevel( level ) );
+ return logger.isEnabled( toLevel( level ) );
}
@Override
public final void log( final StackTraceElement caller, final MessageLevel
messageLevel, final String messageId, final String messageText, final Throwable
thrown ) {
- LoggingEvent event = new LoggingEvent(
- /* String fqnOfCategoryClass */ CLASS_NAME,
- /* Category logger */ logger,
- /* long timeStamp */ System.currentTimeMillis(),
- /* Level level */ toLevel( messageLevel ),
- /* Object message */ messageText,
- /* String threadName */ Thread.currentThread().getName(),
- /* ThrowableInformation throwable */ toThrownInformation( thrown ),
- /* String ndc */ null,
- /* LocationInfo info */ toLocationInfo( caller ),
- /* java.util.Map properties */ null );
- logger.callAppenders( event );
- }
-
- private static ThrowableInformation toThrownInformation( final Throwable
thrown ) {
- ThrowableInformation info = null;
- if( thrown != null ) {
- info = new ThrowableInformation( thrown );
- }
- return info;
- }
-
- private static LocationInfo toLocationInfo( final StackTraceElement caller )
{
- LocationInfo info = null;
- if( caller != null ) {
- info = new LocationInfo( caller.getFileName(), caller.getClassName(),
caller.getMethodName(), Integer.toString(caller.getLineNumber()) );
- }
- return info;
+ logger.logMessage(toLevel(messageLevel), null, CLASS_NAME, caller, new
SimpleMessage(messageText), thrown);
Review comment:
Is there a before/after example of what these log messages look like?
Are they similar/different?
##########
File path: gateway-release/home/conf/gateway-log4j.properties
##########
@@ -1,85 +0,0 @@
-# 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.
-
-app.log.dir=${launcher.dir}/../logs
-app.log.file=${launcher.name}.log
-app.audit.file=${launcher.name}-audit.log
-
-log4j.rootLogger=ERROR, drfa
-
-log4j.logger.org.apache.knox.gateway=INFO
-#log4j.logger.org.apache.knox.gateway=DEBUG
-
-#log4j.logger.org.eclipse.jetty=DEBUG
-#log4j.logger.org.apache.shiro=DEBUG
-#log4j.logger.org.apache.http=DEBUG
-#log4j.logger.org.apache.http.client=DEBUG
-#log4j.logger.org.apache.http.headers=DEBUG
-#log4j.logger.org.apache.http.wire=DEBUG
-
-log4j.appender.stdout=org.apache.log4j.ConsoleAppender
-log4j.appender.stdout.layout=org.apache.log4j.PatternLayout
-log4j.appender.stdout.layout.ConversionPattern=%d{yy/MM/dd HH:mm:ss} %p %c{2}:
%m%n
-
-log4j.appender.drfa=org.apache.log4j.DailyRollingFileAppender
-log4j.appender.drfa.File=${app.log.dir}/${app.log.file}
-log4j.appender.drfa.DatePattern=.yyyy-MM-dd
-log4j.appender.drfa.layout=org.apache.log4j.PatternLayout
-log4j.appender.drfa.layout.ConversionPattern=%d{ISO8601} %-5p %c{2}
(%F:%M(%L)) - %m%n
-
-log4j.logger.audit=INFO, auditfile
-log4j.appender.auditfile=org.apache.log4j.DailyRollingFileAppender
-log4j.appender.auditfile.File=${app.log.dir}/${app.audit.file}
-log4j.appender.auditfile.Append = true
-log4j.appender.auditfile.DatePattern = '.'yyyy-MM-dd
-log4j.appender.auditfile.layout =
org.apache.knox.gateway.audit.log4j.layout.AuditLayout
-
-#log4j.logger.org.apache.knox.gateway.access=TRACE,httpaccess
-#log4j.additivity.org.apache.knox.gateway.access=false
-
-#log4j.logger.org.apache.knox.gateway.http=TRACE,httpserver
-#log4j.additivity.org.apache.knox.gateway.http=false
-##log4j.logger.org.apache.knox.gateway.http.request.headers=OFF
-##log4j.logger.org.apache.knox.gateway.http.response.headers=OFF
-##log4j.logger.org.apache.knox.gateway.http.request.body=OFF
-##log4j.logger.org.apache.knox.gateway.http.response.body=OFF
-
-#log4j.logger.org.apache.http.wire=DEBUG,httpclient
-#log4j.additivity.org.apache.http.wire=false
-
-#log4j.appender.httpaccess=org.apache.log4j.DailyRollingFileAppender
-#log4j.appender.httpaccess.File=${app.log.dir}/${launcher.name}-http-access.log
-#log4j.appender.httpaccess.DatePattern=.yyyy-MM-dd
-#log4j.appender.httpaccess.layout=org.apache.log4j.PatternLayout
-#log4j.appender.httpaccess.layout.ConversionPattern=%d{ISO8601}|%t|%m%n
-
-#log4j.appender.httpserver=org.apache.log4j.DailyRollingFileAppender
-#log4j.appender.httpserver.File=${app.log.dir}/${launcher.name}-http-server.log
-#log4j.appender.httpserver.DatePattern=.yyyy-MM-dd
-#log4j.appender.httpserver.layout=org.apache.log4j.PatternLayout
-#log4j.appender.httpserver.layout.ConversionPattern=%d{ISO8601}|%t|%m%n
-
-#log4j.appender.httpclient=org.apache.log4j.DailyRollingFileAppender
-#log4j.appender.httpclient.File=${app.log.dir}/${launcher.name}-http-client.log
-#log4j.appender.httpclient.DatePattern=.yyyy-MM-dd
-#log4j.appender.httpclient.layout=org.apache.log4j.PatternLayout
-#log4j.appender.httpclient.layout.ConversionPattern=%d{ISO8601}|%t|%m%n
-
-# Apache Shiro Related logging - KNOX-757
-#log4j.logger.org.springframework=DEBUG
-#log4j.logger.net.sf.ehcache=DEBUG
-#log4j.logger.org.apache.shiro.util.ThreadContext=DEBUG
Review comment:
These are missing from corresponding gateway-log4j2.xml file.
##########
File path:
gateway-provider-rewrite/src/test/java/org/apache/knox/gateway/filter/rewrite/api/UrlRewriteRulesDescriptorFactoryTest.java
##########
@@ -98,10 +99,10 @@ public void testLoadMissingFile() throws IOException {
@Test
public void testLoadEmptyFile() {
- Logger logger = org.apache.log4j.LogManager.getLogger(
"org.apache.commons.digester3.Digester" );
+ Logger logger = (Logger)LogManager.getLogger(
"org.apache.commons.digester3.Digester" );
Review comment:
Why are the casts to `(Logger)` needed? `LogManager.getLogger` looks
like it should return a Logger instance?
##########
File path:
gateway-provider-ha/src/test/java/org/apache/knox/gateway/ha/provider/impl/KafkaZookeeperURLManagerTest.java
##########
@@ -44,6 +44,7 @@
@Before
public void setUp() throws Exception {
+ System.setProperty("zookeeper.jmx.log4j.disable", "true");
Review comment:
This property isn't cleared in tearDown?
This could affect other running tests in the jvm as well.
##########
File path:
gateway-server/src/test/java/org/apache/knox/gateway/services/security/impl/ZookeeperRemoteAliasMonitorTest.java
##########
@@ -69,6 +69,8 @@
@BeforeClass
public static void setupSuite() throws Exception {
+ System.setProperty("zookeeper.jmx.log4j.disable", "true");
Review comment:
This property isn't cleared in tearDown?
This could affect other running tests in the jvm as well.
##########
File path: gateway-server/src/test/resources/log4j.properties
##########
@@ -1,35 +0,0 @@
-##########################################################################
-# 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.
-##########################################################################
-
-#log4j.rootLogger=DEBUG, stdout
-log4j.rootLogger=ERROR, stdout
-
-log4j.appender.stdout=org.apache.log4j.ConsoleAppender
-log4j.appender.stdout.layout=org.apache.log4j.PatternLayout
-log4j.appender.stdout.layout.ConversionPattern=%5p [%c] %m%n
-
-log4j.logger.audit = INFO, collectappender
-log4j.appender.collectappender = org.apache.knox.test.log.CollectAppender
-
-#log4j.logger.org.apache.knox.gateway=DEBUG
-#log4j.logger.org.eclipse.jetty=DEBUG
-#log4j.logger.org.apache.shiro=DEBUG
-#log4j.logger.org.apache.http=DEBUG
-#log4j.logger.org.apache.http.wire=DEBUG
-#log4j.logger.org.apache.http.client=DEBUG
Review comment:
These are missing from log4j2-test.xml equivalent.
##########
File path: gateway-provider-rewrite/src/test/resources/log4j.properties
##########
@@ -1,37 +0,0 @@
-##########################################################################
-# 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.
-##########################################################################
-
-log4j.rootLogger=ERROR, stdout
-
-log4j.appender.stdout=org.apache.log4j.ConsoleAppender
-log4j.appender.stdout.layout=org.apache.log4j.PatternLayout
-log4j.appender.stdout.layout.ConversionPattern=%5p [%c] %m%n
-
-#log4j.logger.org.apache.knox.gateway=DEBUG
-#log4j.logger.org.apache.knox.gateway.http=TRACE
-#log4j.logger.org.apache.knox.gateway.http.request.body=OFF
-#log4j.logger.org.apache.knox.gateway.http.response.body=OFF
-
-#log4j.logger.org.apache.directory=DEBUG
-#log4j.logger.org.eclipse.jetty=DEBUG
-#log4j.logger.org.apache.shiro=DEBUG
-#log4j.logger.org.apache.http=DEBUG
-#log4j.logger.org.apache.http.headers=DEBUG
-#log4j.logger.org.apache.http.wire=DEBUG
-#log4j.logger.org.apache.http.client=DEBUG
Review comment:
These are missing from the corresponding log4j2.xml. They are commented
out since they are commonly used for debugging tests and easier to uncomment
than figure out what to add again.
##########
File path:
gateway-service-remoteconfig/src/test/java/org/apache/knox/gateway/service/config/remote/zk/RemoteConfigurationRegistryClientServiceTestBase.java
##########
@@ -54,6 +54,8 @@
protected TestingCluster setupAndStartSecureTestZooKeeper(String
principal, String digestPassword) throws Exception {
final boolean applyAuthentication = (principal != null);
+ System.setProperty("zookeeper.jmx.log4j.disable", "true");
Review comment:
This property isn't cleared in tearDown?
This could affect other running tests in the jvm as well.
##########
File path:
gateway-test/src/test/java/org/apache/knox/gateway/topology/monitor/RemoteConfigurationMonitorTest.java
##########
@@ -155,6 +155,8 @@ private static File setupDigestSaslConfig(String username,
String password) thro
* Configure and start the ZooKeeper test cluster, and create the znodes
monitored by the RemoteConfigurationMonitor.
*/
private void configureAndStartZKCluster() throws Exception {
+ System.setProperty("zookeeper.jmx.log4j.disable", "true");
Review comment:
This property isn't cleared in tearDown?
This could affect other running tests in the jvm as well.
##########
File path: pom.xml
##########
@@ -234,8 +234,7 @@
<junit.version>4.13.1</junit.version>
<lang-tag.version>1.5</lang-tag.version>
<libpam4j.version>1.11</libpam4j.version>
- <log4j.version>1.2.17</log4j.version>
- <log4j2.version>2.14.0</log4j2.version>
+ <log4j.version>2.14.0</log4j.version>
Review comment:
It would probably make sense to leave this as `log4j2.version` to avoid
any confusion.
##########
File path:
gateway-server/src/test/java/org/apache/knox/gateway/descriptor/xml/XmlGatewayDescriptorImporterTest.java
##########
@@ -153,7 +154,7 @@ public void testXmlGatewayDescriptorLoadInvalid() throws
IOException {
Reader reader = new StringReader( xml );
// Keep the tests quiet. Ignore the stack trace that ends up being
written to System.out.
- Logger logger = Logger.getLogger( "org.apache.commons.digester3.Digester"
);
+ Logger logger = (Logger)LogManager.getLogger(
"org.apache.commons.digester3.Digester" );
Review comment:
Why is `(Logger)` cast needed here?
##########
File path:
gateway-util-common/src/main/java/org/apache/knox/gateway/audit/api/CorrelationService.java
##########
@@ -23,19 +23,6 @@
* Manipulates the correlations context associated with the current thread.
*/
public interface CorrelationService {
-
- /**
- * The recommended protocol header name used to transmit the correlation
context over the network.
- */
- String PROTOCOL_HEADER = "X-Correlation-Context";
Review comment:
I don't know if this is used anywhere - but does this mean we don't send
this across the network anywhere?
##########
File path: gateway-server/src/test/resources/logging.properties
##########
@@ -1,87 +0,0 @@
-##########################################################################
-# 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.
-##########################################################################
-
-############################################################
-# Default Logging Configuration File
-#
-# You can use a different file by specifying a filename
-# with the java.util.logging.config.file system property.
-# For example java -Djava.util.logging.config.file=myfile
-############################################################
Review comment:
There is no replacement for this file? Does that change behavior?
##########
File path: gateway-test/src/test/resources/log4j.properties
##########
@@ -1,44 +0,0 @@
-##########################################################################
-# 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.
-##########################################################################
-
-#log4j.rootLogger=DEBUG, stdout
-log4j.rootLogger=ERROR, stdout
-
-log4j.appender.stdout=org.apache.log4j.ConsoleAppender
-log4j.appender.stdout.layout=org.apache.log4j.PatternLayout
-log4j.appender.stdout.layout.ConversionPattern=%5p [%c] %m%n
-
-log4j.logger.audit = INFO, collectappender
-log4j.appender.collectappender = org.apache.knox.test.log.CollectAppender
-
-#log4j.logger.org.apache.knox.gateway=DEBUG
-#log4j.logger.org.apache.knox.test=DEBUG
-#log4j.logger.org.apache.knox.gateway.http=TRACE
-#log4j.logger.org.apache.knox.gateway.http.request.body=OFF
-#log4j.logger.org.apache.knox.gateway.http.response.body=OFF
-
-#log4j.logger.org.apache.directory=DEBUG
-#log4j.logger.org.eclipse.jetty=DEBUG
-#log4j.logger.org.apache.shiro=DEBUG
-#log4j.logger.org.apache.http=DEBUG
-#log4j.logger.org.apache.http.headers=DEBUG
-#log4j.logger.org.apache.http.wire=DEBUG
-#log4j.logger.org.apache.http.client=DEBUG
-
-# Hide verbose logging - see KNOX-1718 for more details
Review comment:
This is missing from corresponding log4j2.xml - these help with
debugging.
--
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: dev-unsubscr...@knox.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Issue Time Tracking
-------------------
Worklog Id: (was: 649258)
Time Spent: 2h 10m (was: 2h)
> Migrate from Log4j 1.x to 2.x
> -----------------------------
>
> Key: KNOX-1462
> URL: https://issues.apache.org/jira/browse/KNOX-1462
> Project: Apache Knox
> Issue Type: Improvement
> Reporter: Kevin Risden
> Assignee: Attila Magyar
> Priority: Blocker
> Fix For: 2.0.0
>
> Time Spent: 2h 10m
> Remaining Estimate: 0h
>
> Assigning to version 1.2.0 to have a discussion about it.
> Log4j 1.x went EOL in August 2015. JDK 9+ support (KNOX-1458) will require
> Log4j 2 since Knox uses MDC for all the audit logging. More details about the
> issue and migration are found here:
> * [https://blogs.apache.org/logging/entry/moving_on_to_log4j_2]
> * [https://logging.apache.org/log4j/2.x/manual/migration.html]
> Items of "concern"
> * MDC migrate to ThreadContext
> * log4j.properties need to migrate to log4j2.xml (or yaml or json)
> Benefits
> * Async logging
> * Dynamic reloading of configuration changes
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
