vy commented on code in PR #3501: URL: https://github.com/apache/logging-log4j2/pull/3501#discussion_r2072434647
########## 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()); + watchMonitorUris(); + } else { + LOGGER.info( + "Start watching for changes to {} every {} seconds", + getConfigurationSource(), + watchManager.getIntervalSeconds()); + } Review Comment: ... and, this is the wrong place and time to do admit monitor URIs to the `WatchManager`. Would you move this admission operation (i.e., the logic in `watchMonitorUris`) to `initializeWatchers`, please? ########## 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: See [this recent discussion on how to handle configuration failures](https://lists.apache.org/thread/h2oydyk6xld47ljttqvflbt4530o73vw). In short, configuration failures should result in the entire configuration to fail, instead of applying in partially. Existing code that doesn't adhere to this needs to be fixed, but that is another issue. ########## 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()); + watchMonitorUris(); + } else { + LOGGER.info( + "Start watching for changes to {} every {} seconds", + getConfigurationSource(), + watchManager.getIntervalSeconds()); + } Review Comment: Can't we simplify this as follows? (Can `uris` be null at all?) ```suggestion LOGGER.info( "Start watching for changes to {} and {} every {} seconds", getConfigurationSource(), uris, watchManager.getIntervalSeconds()); watchMonitorUris(); ``` -- 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