-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
HADOOP-18088. Replace log4j 1.x with reload4j. #4052
Conversation
Tarball built by
Running tutorial code on |
💔 -1 overall
This message was automatically generated. |
803bef4
to
aa7054b
Compare
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking how to make it simpler. A lot of the exclusion rules are similar, and can be simplified by the DependencyManagement in hadoop-project/pom.xml.
@jojochuang I updated the patch based on your suggestion. The tarball built by |
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Apache9 what do you think? It looks mostly okay to me but HBase fails to build because reload4j is not permitted in the HBase dependency. This PR is targeting Hadoop 3.3.3.
It is fine. In HBase will exclude the log4j related jars while depending on hadoop, after the change here, we just need to add the reload4j related jars to the exclusions section too. |
@Apache9 looking at this in 3.3.x too, FYI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; couple of comments need to remove their references to log4j just to avoid confusion
@@ -151,7 +151,7 @@ | |||
<!-- Leave commons-logging unshaded so downstream users can configure logging. --> | |||
<exclude>commons-logging:commons-logging</exclude> | |||
<!-- Leave log4j unshaded so downstream users can configure logging. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: comment needs updating
@@ -85,7 +85,7 @@ | |||
<!-- Leave commons-logging unshaded so downstream users can configure logging. --> | |||
<exclude>commons-logging:commons-logging</exclude> | |||
<!-- Leave log4j unshaded so downstream users can configure logging. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: comment needs updating
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the comments. Thanks, @steveloughran.
💔 -1 overall
This message was automatically generated. |
Co-authored-by: Masatake Iwasaki <iwasakims@apache.org>
…f hadoop-project POM. Co-authored-by: Masatake Iwasaki <iwasakims@apache.org>
3b3eadf
to
a375ff4
Compare
I found that we need to update assembly definition under hadoop-assemblies too. I filed HADOOP-18192(#4136) for branch-3.2. I added equivalent fix for branch-3.3 here in the last commit. @steveloughran |
|
good catch about assemblies. i think you can use findclass to identify which jar a log4j or slf4j class is loaded from,just as i have done for the 1.2 classes |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly OK. The most important thing is to add the enforcer rules to ban log4j and slf4j-log4j12 dependencies.
@@ -89,7 +89,7 @@ | |||
<!-- Leave commons-logging unshaded so downstream users can configure logging. --> | |||
<exclude>commons-logging:commons-logging</exclude> | |||
<!-- Leave log4j unshaded so downstream users can configure logging. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log4j -> reload4j
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
hadoop-project/pom.xml
Outdated
@@ -81,8 +81,8 @@ | |||
<httpcore.version>4.4.13</httpcore.version> | |||
|
|||
<!-- SLF4J/LOG4J version --> | |||
<slf4j.version>1.7.30</slf4j.version> | |||
<log4j.version>1.2.17</log4j.version> | |||
<slf4j.version>1.7.35</slf4j.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.7.36 is out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diff from 1.7.35 looks safe and good to upgrade here. I bumped the version.
2022-02-08 - Release of SLF4J 1.7.36
Correct corrupt "Export-Package" declaration in MANIFEST.MF in log4j-over-slf4j module. This fixes SLF4J-541 reported by Flavio Donzé.Starting with version 1.7.36, slf4j releases will be reproducible. By reproducible we mean that anyone checking out the code corresponding to the release version from source code repository and building that local copy, will obtain an identical binary to the published binary.
@@ -2167,6 +2269,9 @@ | |||
<exclude>com.sun.jersey.jersey-test-framework:*</exclude> | |||
<exclude>com.google.inject:guice</exclude> | |||
<exclude>org.ow2.asm:asm</exclude> | |||
|
|||
<exclude>org.slf4j:slf4j-log4j12</exclude> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good.
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
are the tests just HDFS playing up? if so, I'm not worried, though I'd hope those test gets fixed. |
I can reproduce the failure of TestOfflineImageViewer#testReverseXmlWithoutSnapshotDiffSection, TestRouterRpc#testSetBalancerBandwidth and TestRouterRpcMultiDestination#testSetBalancerBandwidth on my local even without the patch. It should be addressed in another JIRA. |
Ok +1 from me |
I merged this. Thanks, @steveloughran, @jojochuang and @Apache9. |
Co-authored-by: Wei-Chiu Chuang <weichiu@apache.org>
Co-authored-by: Wei-Chiu Chuang <weichiu@apache.org>
Co-authored-by: Wei-Chiu Chuang <weichiu@apache.org>
Co-authored-by: Wei-Chiu Chuang <weichiu@apache.org>
<groupId>org.apache.hbase</groupId> | ||
<artifactId>hbase-server</artifactId> | ||
<version>${hbase.version}</version> | ||
<exclusions> | ||
<exclusion> | ||
<groupId>log4j</groupId> | ||
<artifactId>log4j</artifactId> | ||
</exclusion> | ||
</exclusions> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.apache.hbase</groupId> | ||
<artifactId>hbase-server</artifactId> | ||
<version>${hbase.version}</version> | ||
<scope>test</scope> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iwasakims hbase-server was already there, with compile scope, why was it added here again with test scope?
It seems to give warnings while building 3.3.3 to me
[WARNING]
[WARNING] Some problems were encountered while building the effective model for org.apache.hadoop:hadoop-project:pom:3.3.3
[WARNING] 'dependencyManagement.dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: org.apache.hbase:hbase-server:jar -> duplicate declaration of version ${hbase.version} @ line 1691, column 19
Change-Id: Id2f25f0b9d26e0ff73f828acbfb4e6fbac3425cc Co-authored-by: Wei-Chiu Chuang <weichiu@apache.org>
Change-Id: I7aa29cdbe66e3eabcff5d30f244c55563a6dfa7f Co-authored-by: Wei-Chiu Chuang <weichiu@apache.org>
Co-authored-by: Wei-Chiu Chuang <weichiu@apache.org> Includes HADOOP-18354. Upgrade reload4j to 1.22.2 due to XXE vulnerability (#4607). Log4j 1.2.17 has been replaced by reloadj 1.22.2 SLF4J is at 1.7.36
Co-authored-by: Wei-Chiu Chuang <weichiu@apache.org> Includes HADOOP-18354. Upgrade reload4j to 1.22.2 due to XXE vulnerability (#4607). Log4j 1.2.17 has been replaced by reloadj 1.22.2 SLF4J is at 1.7.36 (cherry picked from commit 095dfcc)
Co-authored-by: Wei-Chiu Chuang <weichiu@apache.org> Includes HADOOP-18354. Upgrade reload4j to 1.22.2 due to XXE vulnerability (#4607). Log4j 1.2.17 has been replaced by reloadj 1.22.2 SLF4J is at 1.7.36 (cherry picked from commit 095dfcc)
No description provided.