Skip to content
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.

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

[BEAM-12422] Drop other optional logger libraries since Netty can detect which logger is available automatically #15113

Merged
merged 1 commit into from
Jul 1, 2021

Conversation

lukecwik
Copy link
Member

@lukecwik lukecwik commented Jul 1, 2021

Also revert existing change that removed this dep and its relocation exclusions

See #15098 (comment) for details as to why this change over removing log4j-api


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

ValidatesRunner compliance status (on master branch)

Lang ULR Dataflow Flink Samza Spark Twister2
Go --- Build Status Build Status --- Build Status ---
Java Build Status Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Python --- Build Status
Build Status
Build Status
Build Status
Build Status
--- Build Status ---
XLang Build Status Build Status Build Status --- Build Status ---

Examples testing status on various runners

Lang ULR Dataflow Flink Samza Spark Twister2
Go --- --- --- --- --- --- ---
Java --- Build Status
Build Status
Build Status
--- --- --- --- ---
Python --- --- --- --- --- --- ---
XLang --- --- --- --- --- --- ---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go Java Python
Build Status Build Status Build Status
Build Status
Build Status

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website Whitespace Typescript
Non-portable Build Status
Build Status
Build Status
Build Status
Build Status
Build Status Build Status Build Status Build Status
Portable --- Build Status Build Status --- --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests

See CI.md for more information about GitHub Actions CI.

@lukecwik
Copy link
Member Author

lukecwik commented Jul 1, 2021

R: @suztomo @iemejia

@codecov
Copy link

codecov bot commented Jul 1, 2021

Codecov Report

Merging #15113 (57a4171) into master (22ec4e6) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 57a4171 differs from pull request most recent head 999a77e. Consider uploading reports for the commit 999a77e to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master   #15113   +/-   ##
=======================================
  Coverage   83.79%   83.80%           
=======================================
  Files         441      441           
  Lines       59530    59530           
=======================================
+ Hits        49886    49891    +5     
+ Misses       9644     9639    -5     
Impacted Files Coverage Δ
...ks/python/apache_beam/runners/worker/sdk_worker.py 89.00% <0.00%> (+0.15%) ⬆️
...eam/runners/interactive/interactive_environment.py 90.70% <0.00%> (+0.37%) ⬆️
sdks/python/apache_beam/utils/interactive_utils.py 95.12% <0.00%> (+7.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22ec4e6...999a77e. Read the comment docs.

@lukecwik
Copy link
Member Author

lukecwik commented Jul 1, 2021

Run Python PreCommit

@suztomo
Copy link
Contributor

suztomo commented Jul 1, 2021

netty uses log4j and commons-logging and is a transitive dep of grpc.

From your comment in https://issues.apache.org/jira/browse/BEAM-12422

Can you give me the Maven coordinates of the netty library you checked?

I tried generating the dependency tree (https://gist.github.com/suztomo/5b906fa4850b4685081e870d45ae26b9) of vendor/grpc-1_36_0 but I don't find them.

@lukecwik
Copy link
Member Author

lukecwik commented Jul 1, 2021

netty uses log4j and commons-logging and is a transitive dep of grpc.

From your comment in https://issues.apache.org/jira/browse/BEAM-12422

Can you give me the Maven coordinates of the netty library you checked?

I tried generating the dependency tree (https://gist.github.com/suztomo/5b906fa4850b4685081e870d45ae26b9) of vendor/grpc-1_36_0 but I don't find them.

https://mvnrepository.com/artifact/io.netty/netty-common/4.1.52.Final

I looked at the source and it seems as though they try a bunch of loggers to find which one exists
https://github.com/netty/netty/blob/5178b2c7a5eb876f06912753e8251836bc3f1051/common/src/main/java/io/netty/util/internal/logging/InternalLoggerFactory.java
so we could close this PR and leave your change as is.

WDYT?

@lukecwik
Copy link
Member Author

lukecwik commented Jul 1, 2021

netty uses log4j and commons-logging and is a transitive dep of grpc.

From your comment in https://issues.apache.org/jira/browse/BEAM-12422
Can you give me the Maven coordinates of the netty library you checked?
I tried generating the dependency tree (https://gist.github.com/suztomo/5b906fa4850b4685081e870d45ae26b9) of vendor/grpc-1_36_0 but I don't find them.

https://mvnrepository.com/artifact/io.netty/netty-common/4.1.52.Final

I looked at the source and it seems as though they try a bunch of loggers to find which one exists
https://github.com/netty/netty/blob/5178b2c7a5eb876f06912753e8251836bc3f1051/common/src/main/java/io/netty/util/internal/logging/InternalLoggerFactory.java
so we could close this PR and leave your change as is.

WDYT?

I decided to go with dropping the logging deps which will help clean-up our deps just a little.

PTAL

@lukecwik lukecwik changed the title [BEAM-12422] Upgrade log4j version not affected by CVE-2017-5645 [BEAM-12422] Drop other optional logger libraries since Netty can detect which logger is available automatically Jul 1, 2021
@suztomo
Copy link
Contributor

suztomo commented Jul 1, 2021

That makes sense. Netty checks the existence of the loggers. The vendored artifact does not need to include all the loggers.

@lukecwik
Copy link
Member Author

lukecwik commented Jul 1, 2021

Run Java_Examples_Dataflow PreCommit

@lukecwik
Copy link
Member Author

lukecwik commented Jul 1, 2021

Run Python PreCommit

@lukecwik lukecwik merged commit 37fd13e into apache:master Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants