Skip to content

SAMZA-2155: Remove log4j log4j2 dependency from samza-test #985

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

Closed
Sanil15 wants to merge 5 commits into from
Closed

SAMZA-2155: Remove log4j log4j2 dependency from samza-test #985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9a14893
SAMZA-2155: Remove log4j log4j2 dependency from samza-test
Sanil15 Apr 4, 2019
a890e4c
Addressing Review, adding slf4j safeguards
Sanil15 Apr 5, 2019
7e7d357
Removing a typo
Sanil15 Apr 5, 2019
b60f35d
Reverting a minor fix
Sanil15 Apr 9, 2019
221e3f4
Merge branch 'master' into SAMZA-2155
Sanil15 Apr 9, 2019
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
5 changes: 3 additions & 2 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -810,6 +810,9 @@ project(":samza-test_$scalaSuffix") {
compile.exclude group: 'javax.jms', module: 'jms'
compile.exclude group: 'com.sun.jdmk', module: 'jmxtools'
compile.exclude group: 'com.sun.jmx', module: 'jmxri'
// Exclude Slf4j bindings for log4j1 and log4j2 since samza-test just logs to console using slf4j-simple
compile.exclude group: 'org.slf4j', module: 'slf4j-log4j12'
compile.exclude group: 'org.apache.logging.log4j', module: 'log4j-slf4j-impl'
}

dependencies {
Expand All @@ -819,8 +822,6 @@ project(":samza-test_$scalaSuffix") {
compile project(":samza-core_$scalaSuffix")
compile project(":samza-kafka_$scalaSuffix")
compile project(":samza-sql_$scalaSuffix")
runtime project(":samza-log4j_$scalaSuffix")
Copy link
Contributor

@prateekm prateekm Apr 4, 2019

Choose a reason for hiding this comment

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

Do you need to exclude the slf4j-log4j12 (log4j1 binding) and slf4j-log4j-impl (log4j2 binding) from the dependencies? At least log4j1 binding currently comes transitively from several of the modules. If SLF4J picks it up and it finds that the log4j.xml is missing, it might not log to console.

Copy link
Contributor

@rmatharu-zz rmatharu-zz Apr 4, 2019

Choose a reason for hiding this comment

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

+1, it'll log to stdout, but thats still not desirable

Copy link
Contributor

@prateekm prateekm Apr 4, 2019

Choose a reason for hiding this comment

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

@rmatharu Actually logging to stdout is what we want, unless you think it'll be better to have log files for tests. For that, we already have a testRuntime dependency on slf4j-simple (console logger). My concern is that it will pick log4j1 binding, find that the xml file is missing, and not log anything (like you saw in the diagnostics repartitioner)

Copy link
Contributor Author

@Sanil15 Sanil15 Apr 5, 2019

Choose a reason for hiding this comment

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

@prateekm yup that safeguard is good to have.

gradle :samza-test:test -i with that safeguard exclude logs to console

runtime project(":samza-log4j2_$scalaSuffix")
runtime project(":samza-yarn_$scalaSuffix")
runtime project(":samza-hdfs_$scalaSuffix")
compile "org.scala-lang:scala-library:$scalaVersion"
Expand Down
50 changes: 0 additions & 50 deletions samza-test/src/main/resources/log4j.xml
View file
Open in desktop

This file was deleted.

41 changes: 0 additions & 41 deletions samza-test/src/main/resources/log4j2.xml
View file
Open in desktop

This file was deleted.