vy commented on code in PR #3501:
URL: https://github.com/apache/logging-log4j2/pull/3501#discussion_r2041190519
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java:
##########
@@ -767,6 +785,18 @@ protected void doConfigure() {
setParents();
}
+ private List<URI> convertToJavaNetUris(final List<Uri> uris) {
+ final List<URI> javaNetUris = new ArrayList<>();
+ for (Uri uri : uris) {
+ try {
+ javaNetUris.add(new URI(uri.getUri()));
+ } catch (URISyntaxException e) {
+ LOGGER.error("Error parsing monitor URI: " + uri, e);
Review 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.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFileWatcher.java:
##########
@@ -28,38 +31,57 @@
*/
public class ConfigurationFileWatcher extends AbstractWatcher implements
FileWatcher {
- private File file;
- private long lastModifiedMillis;
+ private Map<File, Long> monitoredFiles = new HashMap<>();
public ConfigurationFileWatcher(
final Configuration configuration,
final Reconfigurable reconfigurable,
final List<ConfigurationListener> configurationListeners,
long lastModifiedMillis) {
super(configuration, reconfigurable, configurationListeners);
- this.lastModifiedMillis = lastModifiedMillis;
}
@Override
public long getLastModified() {
- return file != null ? file.lastModified() : 0;
+ Long lastModifiedMillis = 0L;
+ for (final File monitoredFile : monitoredFiles.keySet()) {
+ if (monitoredFile.lastModified() > lastModifiedMillis) {
+ lastModifiedMillis = monitoredFile.lastModified();
+ }
+ }
+ return lastModifiedMillis;
}
@Override
public void fileModified(final File file) {
- lastModifiedMillis = file.lastModified();
+ monitoredFiles.entrySet().stream()
+ .forEach(monitoredFile ->
+
monitoredFile.setValue(monitoredFile.getKey().lastModified()));
Review Comment:
Shouldn't you be `monitoredFiles.get(file)` and then...?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java:
##########
@@ -323,10 +325,19 @@ public void start() {
LOGGER.info("Starting configuration {}...", this);
this.setStarting();
if (watchManager.getIntervalSeconds() >= 0) {
- LOGGER.info(
- "Start watching for changes to {} every {} seconds",
- getConfigurationSource(),
- watchManager.getIntervalSeconds());
+ if (uris != null && uris.size() > 0) {
+ LOGGER.info(
+ "Start watching for changes to {} and {} every {}
seconds",
+ getConfigurationSource(),
+ uris,
+ watchManager.getIntervalSeconds());
+ watchManager.addMonitorUris(configurationSource.getSource(),
uris);
Review Comment:
Can't we instead for each URI create a new `ConfigurationFileWatcher` and
register it using `watchManager.watch(cfgSource, watcher)`?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFileWatcher.java:
##########
@@ -28,38 +31,57 @@
*/
public class ConfigurationFileWatcher extends AbstractWatcher implements
FileWatcher {
- private File file;
- private long lastModifiedMillis;
+ private Map<File, Long> monitoredFiles = new HashMap<>();
public ConfigurationFileWatcher(
final Configuration configuration,
final Reconfigurable reconfigurable,
final List<ConfigurationListener> configurationListeners,
long lastModifiedMillis) {
super(configuration, reconfigurable, configurationListeners);
- this.lastModifiedMillis = lastModifiedMillis;
}
@Override
public long getLastModified() {
- return file != null ? file.lastModified() : 0;
+ Long lastModifiedMillis = 0L;
+ for (final File monitoredFile : monitoredFiles.keySet()) {
+ if (monitoredFile.lastModified() > lastModifiedMillis) {
+ lastModifiedMillis = monitoredFile.lastModified();
+ }
+ }
+ return lastModifiedMillis;
}
@Override
public void fileModified(final File file) {
- lastModifiedMillis = file.lastModified();
+ monitoredFiles.entrySet().stream()
+ .forEach(monitoredFile ->
+
monitoredFile.setValue(monitoredFile.getKey().lastModified()));
}
@Override
public void watching(final Source source) {
- file = source.getFile();
- lastModifiedMillis = file.lastModified();
+ File file = source.getFile();
+ monitoredFiles.put(file, file.lastModified());
super.watching(source);
}
+ /**
+ * Add the given URIs to be watched.
+ *
+ * @param monitorUris URIs to also watch
+ */
+ public void addMonitorUris(final List<URI> monitorUris) {
+ monitorUris.forEach(uri -> {
+ File additionalFile = new Source(uri).getFile();
+ monitoredFiles.put(additionalFile, additionalFile.lastModified());
+ });
+ }
Review 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?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFileWatcher.java:
##########
@@ -28,38 +31,57 @@
*/
public class ConfigurationFileWatcher extends AbstractWatcher implements
FileWatcher {
- private File file;
- private long lastModifiedMillis;
+ private Map<File, Long> monitoredFiles = new HashMap<>();
public ConfigurationFileWatcher(
final Configuration configuration,
final Reconfigurable reconfigurable,
final List<ConfigurationListener> configurationListeners,
long lastModifiedMillis) {
super(configuration, reconfigurable, configurationListeners);
- this.lastModifiedMillis = lastModifiedMillis;
}
@Override
public long getLastModified() {
- return file != null ? file.lastModified() : 0;
+ Long lastModifiedMillis = 0L;
+ for (final File monitoredFile : monitoredFiles.keySet()) {
+ if (monitoredFile.lastModified() > lastModifiedMillis) {
+ lastModifiedMillis = monitoredFile.lastModified();
+ }
+ }
+ return lastModifiedMillis;
Review Comment:
I think this can be simplified with a `stream()` and `min()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
