Add option to provide URIs to monitor in addition to the config file #3501
Conversation
vy
added
api
There was a problem hiding this comment.
@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! |
There was a problem hiding this comment.
@MichaelMorrisEst, I am extremely thankful for your effort and patience. I've some questions. I'd appreciate it if you can address them.
| monitoredFiles.entrySet().stream() | ||
| .forEach(monitoredFile -> | ||
| monitoredFile.setValue(monitoredFile.getKey().lastModified())); |
There was a problem hiding this comment.
Shouldn't you be monitoredFiles.get(file) and then...?
There was a problem hiding this comment.
Not applicable any more with changes from other comments
| Long lastModifiedMillis = 0L; | ||
| for (final File monitoredFile : monitoredFiles.keySet()) { | ||
| if (monitoredFile.lastModified() > lastModifiedMillis) { | ||
| lastModifiedMillis = monitoredFile.lastModified(); | ||
| } | ||
| } | ||
| return lastModifiedMillis; |
There was a problem hiding this comment.
I think this can be simplified with a stream() and min().
There was a problem hiding this comment.
Not applicable any more with changes from other comments
| public void addMonitorUris(final List<URI> monitorUris) { | ||
| monitorUris.forEach(uri -> { | ||
| File additionalFile = new Source(uri).getFile(); | ||
| monitoredFiles.put(additionalFile, additionalFile.lastModified()); | ||
| }); | ||
| } |
There was a problem hiding this comment.
I'm not really sure if ConfigurationFW should have a concept of monitor URIs. Instead, the call-site can call watching(Source) multiple times, but this time super.watching(source) will be overriden incorrectly. I have the impression that there is an impedance mismatch. That is, we're trying to extend CFW to support multiple files. Instead, in AbstractConfiguration, can't we create a CFW for each monitor URI?
There was a problem hiding this comment.
Yes that works too, updated accordingly
| try { | ||
| javaNetUris.add(new URI(uri.getUri())); | ||
| } catch (URISyntaxException e) { | ||
| LOGGER.error("Error parsing monitor URI: " + uri, e); |
There was a problem hiding this comment.
Are we sure we want to proceed with (re)configuration obtained from an invalid configuration? I think we should propagate the exception and let it fail.
There was a problem hiding this comment.
My thinking here was to be consistent with other error scenarios, for example lines 726, 764, 790.
Also to let the exception propagate we would need to declare it on the doConfigure() method which could cause an impact on anyone with their own custom implementation which extends AbstractConfiguration
| getConfigurationSource(), | ||
| uris, | ||
| watchManager.getIntervalSeconds()); | ||
| watchManager.addMonitorUris(configurationSource.getSource(), uris); |
There was a problem hiding this comment.
Can't we instead for each URI create a new ConfigurationFileWatcher and register it using watchManager.watch(cfgSource, watcher)?
There was a problem hiding this comment.
Yes that works too, updated accordingly
Generated by gradle/develocity-actions |
A change detected in either the config file itself or the provided additional URIs shall result in a reconfigure Signed-off-by: MichaelMorris <[email protected]>
…ribute of the Configuration element Signed-off-by: MichaelMorris <[email protected]>
Signed-off-by: MichaelMorris <[email protected]>
MichaelMorrisEst
force-pushed
the
issue-3074
branch
from
ada292f to
98cad4f
Compare
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