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

Reply via email to