Skip to content

[HUDI-4441] Log4j2 configuration fixes and removal of log4j1 dependencies #6170

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
xushiyan merged 47 commits into from
Aug 25, 2022
Merged

[HUDI-4441] Log4j2 configuration fixes and removal of log4j1 dependencies #6170

Show file tree
Hide file tree
Changes from 44 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
185a60e
try adding in propery enabling bridge
the-other-tim-brown Jul 21, 2022
956a4b0
fix license issue
the-other-tim-brown Jul 21, 2022
ac9ea80
configure scalatest plugin in parent pom, use log4j properties for sc…
the-other-tim-brown Jul 21, 2022
2a317eb
try setting log4j config in spark conf
the-other-tim-brown Jul 21, 2022
8669088
try setting log4j1 compatibility
the-other-tim-brown Jul 21, 2022
c7f4397
try adding in enforcer
the-other-tim-brown Jul 22, 2022
7f37493
add in enforcer, update some poms' included dependencies
the-other-tim-brown Jul 22, 2022
d3c9266
more pom updates
the-other-tim-brown Jul 22, 2022
a0f4695
try to fix bot conf update
the-other-tim-brown Jul 22, 2022
2d1028e
fix exclusions
the-other-tim-brown Jul 24, 2022
a3cc6e4
remove component properties
the-other-tim-brown Jul 24, 2022
b0de1cd
replace all properties files with log4j2 equivalents
the-other-tim-brown Jul 24, 2022
79c02b0
try to denoise build more, add in jul-to-slf4j for hudi-cli module
the-other-tim-brown Jul 24, 2022
1a2d20c
remove test scope on logging dep
the-other-tim-brown Jul 24, 2022
dbac26f
update hudi-spark logging deps
the-other-tim-brown Jul 24, 2022
34485e3
prevent potential race condition for removing appenders by making sur…
the-other-tim-brown Jul 25, 2022
16ff6fb
update log properties in setup_demo_container.sh, remove ignored opti…
the-other-tim-brown Jul 25, 2022
c7481b6
change jul dep
the-other-tim-brown Jul 25, 2022
6bb9f9a
Revert "change jul dep"
the-other-tim-brown Jul 25, 2022
111dd8b
use common test module, single surefire properties definition
the-other-tim-brown Jul 25, 2022
943826e
Merge branch 'apache:master' into HUDI-4441-logging-test
the-other-tim-brown Jul 26, 2022
95500ad
fix logger test
the-other-tim-brown Jul 26, 2022
1de7212
add in options to azure build as well to reduce noise
the-other-tim-brown Jul 26, 2022
e3a54b8
fix surefire groups error
the-other-tim-brown Jul 26, 2022
9b3abc2
hudi-spark-client add in jul bridge
the-other-tim-brown Jul 26, 2022
69f96ea
add bridge for hudi-spark2 module
the-other-tim-brown Jul 26, 2022
9439176
temporary: change hudi logs to warning to more easily see error in ci
the-other-tim-brown Jul 26, 2022
a329180
Revert "temporary: change hudi logs to warning to more easily see err…
the-other-tim-brown Jul 26, 2022
644e763
change default scope of logging
the-other-tim-brown Aug 11, 2022
e9a0d90
Merge remote-tracking branch 'origin' into HUDI-4441-logging-test
the-other-tim-brown Aug 11, 2022
b2d6b01
update version for test module
the-other-tim-brown Aug 11, 2022
ab07f13
remove log4j2.properties in favor of letting user provide them, move …
the-other-tim-brown Aug 17, 2022
53c4195
add in slf4j impl as test dependency for hudi-spark-client
the-other-tim-brown Aug 17, 2022
a71b8d7
add log file location to args
the-other-tim-brown Aug 17, 2022
2b94083
fix cli arg name for logging config location
the-other-tim-brown Aug 17, 2022
fe71d7f
try to resolve org.slf4j.impl.StaticLoggerBinder not found issues
the-other-tim-brown Aug 18, 2022
c272a3a
try changing log bridge scopes
the-other-tim-brown Aug 18, 2022
e1e4c74
Revert "try changing log bridge scopes"
the-other-tim-brown Aug 18, 2022
5963b23
try adding log bridges to test module with new scope
the-other-tim-brown Aug 18, 2022
4b92656
set log level to warn for hudi in surefire-quiet properties
the-other-tim-brown Aug 19, 2022
90e2512
revert mvn logging approach
the-other-tim-brown Aug 19, 2022
625d6c7
fix issues with log level requirement in tests
the-other-tim-brown Aug 19, 2022
ea168c8
clean up scopes
the-other-tim-brown Aug 19, 2022
555cc3c
one more cleanup
the-other-tim-brown Aug 19, 2022
7330577
remove bridge impls from everywhere but hudi-common
the-other-tim-brown Aug 19, 2022
1b5f474
more logging dependency cleanup
the-other-tim-brown Aug 19, 2022
520b1b5
remove extra logging dependency
the-other-tim-brown Aug 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions .github/workflows/bot.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ on:
branches:
- master
- 'release-*'
env:
MVN_ARGS: -ntp -B -V -Pwarn-log -Dorg.slf4j.simpleLogger.log.org.apache.maven.plugins.shade=warn -Dorg.slf4j.simpleLogger.log.org.apache.maven.plugins.dependency=warn
Copy link
Contributor

@alexeykudinkin alexeykudinkin Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add these overrides to the config instead of needing to specify them here?

Copy link
Contributor Author

@the-other-tim-brown the-other-tim-brown Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the shade and dependency plugin, I didn't find a way to specify the log level in the configuration of the plugin unfortunately. We could put these in a basic logging configuration file that maven uses as well though.

For the -ntp -B -V, that will need to be here

Copy link
Contributor

@alexeykudinkin alexeykudinkin Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. You're saying if you specify the logs it in props for these loggers it's not not picked up?

Copy link
Contributor Author

@the-other-tim-brown the-other-tim-brown Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can specify a logging configuration for maven itself if we want and configure the simple logger there. Some plugins allow you to set the log level in their configuration but these plugins do not have that option

Copy link
Contributor

@alexeykudinkin alexeykudinkin Aug 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be preferred

Copy link
Contributor Author

@the-other-tim-brown the-other-tim-brown Aug 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried debugging this for a couple of hours but it looks like maven will not pick up our properties file so I am going to revert back to manually specifying the strings in the build file


jobs:
build:
Expand Down Expand Up @@ -54,19 +56,19 @@ jobs:
SPARK_PROFILE: ${{ matrix.sparkProfile }}
FLINK_PROFILE: ${{ matrix.flinkProfile }}
run:
mvn clean install -Pintegration-tests -D"$SCALA_PROFILE" -D"$SPARK_PROFILE" -D"$FLINK_PROFILE" -DskipTests=true -B -V
mvn clean install -Pintegration-tests -D"$SCALA_PROFILE" -D"$SPARK_PROFILE" -D"$FLINK_PROFILE" -DskipTests=true $MVN_ARGS
- name: Quickstart Test
env:
SCALA_PROFILE: ${{ matrix.scalaProfile }}
SPARK_PROFILE: ${{ matrix.sparkProfile }}
FLINK_PROFILE: ${{ matrix.flinkProfile }}
run:
mvn test -Punit-tests -D"$SCALA_PROFILE" -D"$SPARK_PROFILE" -D"$FLINK_PROFILE" -DfailIfNoTests=false -pl hudi-examples/hudi-examples-flink,hudi-examples/hudi-examples-java,hudi-examples/hudi-examples-spark
mvn test -Punit-tests -D"$SCALA_PROFILE" -D"$SPARK_PROFILE" -D"$FLINK_PROFILE" -DfailIfNoTests=false -pl hudi-examples/hudi-examples-flink,hudi-examples/hudi-examples-java,hudi-examples/hudi-examples-spark $MVN_ARGS
- name: Spark SQL Test
env:
SCALA_PROFILE: ${{ matrix.scalaProfile }}
SPARK_PROFILE: ${{ matrix.sparkProfile }}
FLINK_PROFILE: ${{ matrix.flinkProfile }}
if: ${{ !endsWith(env.SPARK_PROFILE, '2.4') }} # skip test spark 2.4 as it's covered by Azure CI
run:
mvn test -Punit-tests -D"$SCALA_PROFILE" -D"$SPARK_PROFILE" -D"$FLINK_PROFILE" '-Dtest=Test*' -pl hudi-spark-datasource/hudi-spark
mvn test -Punit-tests -D"$SCALA_PROFILE" -D"$SPARK_PROFILE" -D"$FLINK_PROFILE" '-Dtest=Test*' -pl hudi-spark-datasource/hudi-spark $MVN_ARGS
4 changes: 2 additions & 2 deletions azure-pipelines.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,9 @@ parameters:

variables:
BUILD_PROFILES: '-Dscala-2.11 -Dspark2 -Dflink1.14'
PLUGIN_OPTS: '-Dcheckstyle.skip=true -Drat.skip=true -Djacoco.skip=true'
PLUGIN_OPTS: '-Dcheckstyle.skip=true -Drat.skip=true -Djacoco.skip=true -ntp -B -V -Pwarn-log -Dorg.slf4j.simpleLogger.log.org.apache.maven.plugins.shade=warn -Dorg.slf4j.simpleLogger.log.org.apache.maven.plugins.dependency=warn'
MVN_OPTS_INSTALL: '-DskipTests $(BUILD_PROFILES) $(PLUGIN_OPTS)'
MVN_OPTS_TEST: '-fae $(BUILD_PROFILES) $(PLUGIN_OPTS)'
MVN_OPTS_TEST: '-fae -Pwarn-log $(BUILD_PROFILES) $(PLUGIN_OPTS)'
SPARK_VERSION: '2.4.4'
HADOOP_VERSION: '2.7'
SPARK_ARCHIVE: spark-$(SPARK_VERSION)-bin-hadoop$(HADOOP_VERSION)
Expand Down
42 changes: 0 additions & 42 deletions docker/demo/config/log4j.properties
View file
Open in desktop

This file was deleted.

60 changes: 60 additions & 0 deletions docker/demo/config/log4j2.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
###
# 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.
###
status = warn
name = HudiConsoleLog

# Set everything to be logged to the console
appender.console.type = Console
appender.console.name = CONSOLE
appender.console.layout.type = PatternLayout
appender.console.layout.pattern = %d{yy/MM/dd HH:mm:ss} %p %c{1}: %m%n

# Root logger level
rootLogger.level = warn
# Root logger referring to console appender
rootLogger.appenderRef.stdout.ref = CONSOLE
Copy link
Contributor

@alexeykudinkin alexeykudinkin Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to specify stderr or would it fallback to stdout when not set?

Copy link
Contributor Author

@the-other-tim-brown the-other-tim-brown Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will fallback to stdout


# Set the default spark-shell log level to WARN. When running the spark-shell, the
# log level for this class is used to overwrite the root logger's log level, so that
# the user can have different defaults for the shell and regular Spark apps.
logger.apache_spark_repl.name = org.apache.spark.repl.Main
logger.apache_spark_repl.level = warn
# Set logging of integration testsuite to INFO level
logger.hudi_integ.name = org.apache.hudi.integ.testsuite
logger.hudi_integ.level = info
# Settings to quiet third party logs that are too verbose
logger.apache_spark_jetty.name = org.spark_project.jetty
logger.apache_spark_jetty.level = warn
logger.apache_spark_jett_lifecycle.name = org.spark_project.jetty.util.component.AbstractLifeCycle
Copy link
Contributor

@alexeykudinkin alexeykudinkin Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it really odd that we need to now come up with the name for this loggers, instead of just identifying them by the package-name

Copy link
Contributor Author

@the-other-tim-brown the-other-tim-brown Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I'm not a fan of using the .properties format. The yaml format is my preference and even the xml seems a bit more natural than the syntax here.

Copy link
Member

@xushiyan xushiyan Aug 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this about allowing users to fine-tune the logging level for different loggers? i felt in Hudi we don't have to maintain these. users should just override for the loggers they need.

Copy link
Contributor Author

@the-other-tim-brown the-other-tim-brown Aug 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is allows for fine grain control over the logging for the sake of the demo. Should we leave this one instance in the repo since it's for a docker demo?

Copy link
Member

@xushiyan xushiyan Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we leave this one instance in the repo since it's for a docker demo?

Sounds good to me.

logger.apache_spark_jett_lifecycle.level = error
logger.apache_spark_repl_imain.name = org.apache.spark.repl.SparkIMain$exprTyper
logger.apache_spark_repl_imain.level = info
logger.apache_spark_repl_iloop.name = org.apache.spark.repl.SparkILoop$SparkILoopInterpreter
logger.apache_spark_repl_iloop.level = info
logger.parquet.name = org.apache.parquet
logger.parquet.level = error
logger.spark.name = org.apache.spark
logger.spark.level = warn
# Disabling Jetty logs
logger.jetty.name = org.apache.hudi.org.eclipse.jetty
logger.jetty.level = error
# SPARK-9183: Settings to avoid annoying messages when looking up nonexistent UDFs in SparkSQL with Hive support
logger.hive_handler.name = org.apache.hadoop.hive.metastore.RetryingHMSHandler
logger.hive_handler.level = fatal
logger.hive_func_registry.name = org.apache.hadoop.hive.ql.exec.FunctionRegistry
logger.hive_func_registry.level = error
2 changes: 1 addition & 1 deletion docker/demo/setup_demo_container.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

echo "Copying spark default config and setting up configs"
cp /var/hoodie/ws/docker/demo/config/spark-defaults.conf $SPARK_CONF_DIR/.
cp /var/hoodie/ws/docker/demo/config/log4j.properties $SPARK_CONF_DIR/.
cp /var/hoodie/ws/docker/demo/config/log4j2.properties $SPARK_CONF_DIR/.
hadoop fs -mkdir -p /var/demo/
hadoop fs -mkdir -p /tmp/spark-events
hadoop fs -copyFromLocal -f /var/hoodie/ws/docker/demo/config /var/demo/.
Expand Down
11 changes: 7 additions & 4 deletions hudi-aws/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,6 @@
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-api</artifactId>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-1.2-api</artifactId>
Expand Down Expand Up @@ -146,6 +142,13 @@
</dependency>

<!-- Test -->
<dependency>
<groupId>org.apache.hudi</groupId>
<artifactId>hudi-tests-common</artifactId>
<version>${project.version}</version>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
Expand Down
25 changes: 0 additions & 25 deletions hudi-aws/src/test/resources/log4j-surefire.properties
View file
Open in desktop

This file was deleted.

20 changes: 9 additions & 11 deletions hudi-cli/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,28 +207,20 @@
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-1.2-api</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>
<groupId>org.slf4j</groupId>
Copy link
Contributor

@alexeykudinkin alexeykudinkin Aug 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add all bridges (log4j1, slf4j, jul) to hoodie-common and remove them from elsewhere to make sure all modules do have them

<artifactId>jul-to-slf4j</artifactId>
</dependency>

<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-slf4j-impl</artifactId>
<scope>test</scope>
Copy link
Contributor

@alexeykudinkin alexeykudinkin Jul 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not package any logging dependencies (should be provided along w/ query engine)

Copy link
Contributor Author

@the-other-tim-brown the-other-tim-brown Aug 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the cli package used exclusively with query engines? I can imagine it being used as a standalone package as well

Copy link
Contributor

@alexeykudinkin alexeykudinkin Aug 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used as standalone

</dependency>

<dependency>
Expand Down Expand Up @@ -330,6 +322,12 @@
</dependency>

<!-- Test -->
<dependency>
<groupId>org.apache.hudi</groupId>
<artifactId>hudi-tests-common</artifactId>
<version>${project.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
Expand Down
29 changes: 0 additions & 29 deletions hudi-cli/src/test/resources/log4j-surefire-quiet.properties
View file
Open in desktop

This file was deleted.

25 changes: 0 additions & 25 deletions hudi-cli/src/test/resources/log4j-surefire.properties
View file
Open in desktop

This file was deleted.

10 changes: 6 additions & 4 deletions hudi-client/hudi-client-common/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,6 @@
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-api</artifactId>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-1.2-api</artifactId>
Expand Down Expand Up @@ -193,6 +189,12 @@
</dependency>

<!-- Test -->
<dependency>
<groupId>org.apache.hudi</groupId>
<artifactId>hudi-tests-common</artifactId>
<version>${project.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
Copy link
Contributor

@alexeykudinkin alexeykudinkin Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should extract these deps as well to "hudi-tests-common". while we're at it

Copy link
Contributor Author

@the-other-tim-brown the-other-tim-brown Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why junit-vintage-engine is being used by any chance? Do we still need to support running junit4 tests in some modules?

I can add in the junit-jupiter-api, junit-jupiter-engine to the common test module

Copy link
Contributor

@alexeykudinkin alexeykudinkin Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. We can try to remove it and see what happens.

Copy link
Member

@xushiyan xushiyan Aug 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still an exception in TestFSUtils where import org.junit.Rule; is used to inject env var, where it's not doable in junit 5, last time i checked.

<artifactId>junit-jupiter-api</artifactId>
Expand Down
14 changes: 11 additions & 3 deletions ...udi-client-common/src/test/java/org/apache/hudi/callback/http/TestCallbackHttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.mockito.junit.jupiter.MockitoExtension;

import java.io.IOException;
import java.util.UUID;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
Expand Down Expand Up @@ -68,17 +69,24 @@ public class TestCallbackHttpClient {
@Mock
StatusLine statusLine;

private Level initialLogLevel;

@BeforeEach
void prepareAppender() {
when(appender.getName()).thenReturn("MockAppender");
when(appender.getName()).thenReturn("MockAppender-" + UUID.randomUUID());
Copy link
Member

@xushiyan xushiyan Aug 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

when(appender.isStarted()).thenReturn(true);
when(appender.isStopped()).thenReturn(false);
((Logger) LogManager.getLogger(HoodieWriteCommitHttpCallbackClient.class)).addAppender(appender);
Logger logger = (Logger) LogManager.getLogger(HoodieWriteCommitHttpCallbackClient.class);
initialLogLevel = logger.getLevel();
logger.setLevel(Level.DEBUG);
logger.addAppender(appender);
}

@AfterEach
void resetMocks() {
((Logger) LogManager.getLogger(HoodieWriteCommitHttpCallbackClient.class)).removeAppender(appender);
Logger logger = (Logger) LogManager.getLogger(HoodieWriteCommitHttpCallbackClient.class);
logger.setLevel(initialLogLevel);
logger.removeAppender(appender);
reset(appender, httpClient, httpResponse, statusLine);
}

Expand Down
Loading