-
- Notifications
You must be signed in to change notification settings - Fork 1.7k
Add option to provide URIs to monitor in addition to the config file #3501
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
Conversation
vy left a comment
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.
@MichaelMorrisEst, thanks so much for putting effort into this, much appreciated! 😍
I've dropped some remarks. I'd appreciate it if you can update the PR with requested changes. I will have some more remarks – e.g., adding docs – but I will share them last.
log4j-core-test/src/test/java/org/apache/logging/log4j/core/LoggerTest.java Outdated Show resolved Hide resolved
log4j-core-test/src/test/java/org/apache/logging/log4j/core/LoggerTest.java Outdated Show resolved Hide resolved
log4j-core-test/src/test/java/org/apache/logging/log4j/core/PropertiesFileConfigTest.java Outdated Show resolved Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java Outdated Show resolved Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/config/builder/api/package-info.java Outdated Show resolved Hide resolved
...core/src/main/java/org/apache/logging/log4j/core/config/builder/impl/BuiltConfiguration.java Outdated Show resolved Hide resolved
| Thanks for your comments @vy. The comment to use a dedicated element instead of an attribute changes the implementation quiet a bit. Please take a look when you have time, feedback gratefully received. Thanks! |
vy left a comment
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.
@MichaelMorrisEst, I am extremely thankful for your effort and patience. I've some questions. I'd appreciate it if you can address them.
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFileWatcher.java Outdated Show resolved Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFileWatcher.java Outdated Show resolved Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFileWatcher.java Outdated Show resolved Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java Outdated Show resolved Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java Outdated Show resolved Hide resolved
ada292f to 98cad4f Compare
vy left a comment
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.
Would you mind also updating configuration.adoc such that
- Just before
Arbiterssection, create aMonitor URIssection. - Link
Monitor URIsand#configuration-attribute-monitorIntervalto each other with sufficient text and explanation.
log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java Outdated Show resolved Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java Outdated Show resolved Hide resolved
A change detected in either the config file itself or the provided additional URIs shall result in a reconfigure Signed-off-by: MichaelMorris <michael.morris@est.tech>
…ribute of the Configuration element Signed-off-by: MichaelMorris <michael.morris@est.tech>
Signed-off-by: MichaelMorris <michael.morris@est.tech>
Signed-off-by: MichaelMorris <michael.morris@est.tech>
024e54c to bf2ad2c Compare | @MichaelMorrisEst, would you mind giving me write access to your fork, please? I want to push some changes and get this merged for |
Hi @vy, I think I gave you access a few months ago for another PR, can you check and let me know if there is a problem |
@MichaelMorrisEst, see the following failure. Am I missing something? ( |
| Ive added you there now @vy , you should get notification |
| @MichaelMorrisEst, nope, I haven't gotten it yet. Can you merge (not rebase!) the upstream |
| Have just seen the invitation in an inbox I wasn't expecting. 😮💨 Pushed my changes. In short, renamed |
Looks good, I've no further remarks |
ppkarwasz left a comment
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.
Thank you all for the work you've put into this contribution — it's shaping up to be a valuable addition for users!
I’ve reviewed the code and it looks great overall. I do have a few concerns about the proposed syntax for the Java Properties configuration format, and I’d love to discuss them further to ensure we’re aligned with the existing conventions and the future goals for 3.x.
log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java Outdated Show resolved Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java Show resolved Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/config/MonitorResource.java Outdated Show resolved Hide resolved
...ain/java/org/apache/logging/log4j/core/config/properties/PropertiesConfigurationBuilder.java Outdated Show resolved Hide resolved
Signed-off-by: MichaelMorris <michael.morris@est.tech>
ppkarwasz left a comment
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.
Thanks for addressing my suggestions on processing the *.properties format!
Your changes cover the newly introduced monitorResources element well. However, there’s another lesser-known configuration element—AsyncWaitStrategyFactory—that’s still unsupported in the Java Properties format.
What do you think about the approach below to handle both elements together?
...ore/src/main/java/org/apache/logging/log4j/core/config/builder/api/ConfigurationBuilder.java Outdated Show resolved Hide resolved
...ain/java/org/apache/logging/log4j/core/config/properties/PropertiesConfigurationBuilder.java Outdated Show resolved Hide resolved
- Use builder pattern for MontiorResource - Handle AsyncWaitStrategyFactory in properties file as well, in a generic way Signed-off-by: MichaelMorris <michael.morris@est.tech>
I played around with it and think it worked out quiet well I think, was able to re-use some existing methods, so I've pushed with the changes |
ppkarwasz left a comment
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.
This looks great! 💯
There is only one task remaining: the removal of the unused MonitorResourceComponentBuilder class.
...n/java/org/apache/logging/log4j/core/config/builder/api/MonitorResourceComponentBuilder.java Outdated Show resolved Hide resolved
Signed-off-by: MichaelMorris <michael.morris@est.tech>
| Created #3703 to fix the commit signatures. (I did not want to force-push into this PR to avoid truncating git and GitHub conversations history.) |
A change detected in either the config file itself or the provided additional URIs shall result in a reconfigure
See #3074
Checklist
2.xbranch if you are targeting Log4j 2; usemainotherwise./mvnw verifysucceeds (if it fails due to code formatting issues reported by Spotless, simply run./mvnw spotless:applyand retry)src/changelog/.2.x.xdirectory