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
[FLINK-16984][tests] Consolidate Log4j2-test.properties #11634
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 4dd50c1 (Sat Apr 04 16:42:43 UTC 2020) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
logger.zookeeper.name = org.apache.zookeeper | ||
logger.zookeeper.level = OFF | ||
logger.I0Itec.name = org.I0Itec | ||
logger.I0Itec.level = OFF |
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 wonder if it makes sense to keep the file around if there's some domain knowledge in those files. I guess for debugging or working on the Kafka tests it is helpful to suppress some log outputs.
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.
In principle, I like the idea of removing a lot of duplicate files.
I see two problems:
- some modules seem to have some module specifics encoded in their logging config (I propose the keep the files for those modules)
- we need to make sure all the developers understand the new mechanism (I guess we should at least send a notice to the dev@ list?)
I would only have special configs in leaf modules. For modules that are depended on you will usually want these logging configs to be propagated. Like for I'm fine with keeping them in kafka and yarn-tests since they disable quite a lot (all of hadoop/zookeeper).
yes, that was my plan. |
Thanks for creating this PR @zentol. From a software engineering standpoint I agree with these changes. It makes sense to reduce duplication. From a workflow perspective I think it worsens the situation a bit (or at least it disrupts the existing workflow). Currently when debugging a test from within the IDE, I jump to the test file, which brings me to the correct module, and then I change the Before doing any changes, I advise to start a discussion on dev about this topic outlining the benefits and drawbacks as well as the implications for the devs. |
Uses the same trick as #11633 to share a test resource across all modules, in this case
log4j2-test.properties
.This PR removes all
log4j2-test.properties
files except inflink-test-utils-junit
, where it is moved to into the main resource directory instead.