-
Notifications
You must be signed in to change notification settings - Fork 9k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
HDFS-17382. Add Apache Log4j Extras Library to Hadoop 3.4 for Enhance… #6584
base: branch-3.4
Are you sure you want to change the base?
Conversation
972b677
to
c43da1a
Compare
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
…d Log Rolling Capabilities Signed-off-by: woosuk.no <lkjs8269@naver.com>
c43da1a
to
7c60bb5
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.
+1 LGTM, failure seems unrelated to the patch.
Thanks @dntjr8096 for the contribution
@dntjr8096 Thanks for the contribution! @dineshchitlangia Thanks for the review! I am not sure if we still need to include content from log4j 1.x version. I believe we should not include it. |
@dineshchitlangia I want to strip dependendencies from hadoop common, rather than add more, as they contaminate all applications downstream and just add more CVEs. Just as we are adding a hadoop-common-zookeeper, maybe we can add a hadoop-common-server where we add server-side stuff only. we may still need to stitch these into hadoop/common/lib for release builds, but they won't be exported from the hadoop-common POM as transitive dependencies. Also, I like to consider 3.3.x as feature complete and we should focus our feature dev on trunk with backports to branch-3.4. non-asf forks can pick what they want... |
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.
commented; ignore what I said above about hadoop common, as this is already hadoop hdfs.
however, we are trying to move off log4j 1 entirely. Can't we focus on that?
<groupId>log4j</groupId> | ||
<artifactId>apache-log4j-extras</artifactId> | ||
<version>${log4j-extras.version}</version> | ||
<exclusions> |
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.
what else comes in as a dependency
<dependency> | ||
<groupId>log4j</groupId> | ||
<artifactId>apache-log4j-extras</artifactId> | ||
<version>${log4j-extras.version}</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.
declare this in hadoop-project pom with version and exclusions, then declare in this file without those. this is to ensure consistent importing across different modules
@steveloughran agreed with moving off log4j1. |
@dineshchitlangia @slfan1989 @steveloughran First of all, thank you for reviewing my PR. I also agree with moving away from log4j1. Patching to log4j2 and backporting if possible would be ideal. However, I needed a way to roll 'hdfs-audit.log' with gzip. Since the patch to log4j2 is still in progress, using logrotate was the only way to achieve gzip in the current situation. I believe there are others who would benefit from this feature at the moment. Even if it's a temporary solution, I would appreciate it if you could consider it positively. Thanks. |
💔 -1 overall
This message was automatically generated. |
I'm reluctant to do this. Can't you just do it locally? |
@dntjr8096 Given the larger context as explained by @steveloughran and considering the fact the this is a temporary solution, I am inclined with Steve on this one. May be we could just post a blog and let folks do it locally instead of adding it to the project and then dealing with removal later. |
…d Log Rolling Capabilities
Description of PR
https://issues.apache.org/jira/browse/HDFS-17382
I previously tried to apply branch 3.3 as a target, but I want to merge to branch 3.4 first. link to previous PR
How was this patch tested?
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?