vy commented on code in PR #3921:
URL: https://github.com/apache/logging-log4j2/pull/3921#discussion_r2423093331
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java:
##########
@@ -333,6 +334,52 @@ public Configuration getConfiguration(
return getConfiguration(loggerContext, name, configLocation);
}
+ /**
+ * Creates a Configuration from multiple configuration URIs.
+ * If multiple URIs are successfully loaded, they will be combined into a
CompositeConfiguration.
+ *
+ * @param loggerContext the logger context (may be null)
+ * @param name the configuration name (may be null)
+ * @param uris the list of configuration URIs (must not be null or empty)
+ * @return a Configuration created from the provided URIs
+ * @throws NullPointerException if uris is null
+ * @throws IllegalArgumentException if uris is empty
+ * @throws ConfigurationException if no valid configuration could be
created
Review Comment:
> 1. **Align all methods to throw on error.**
> This approach would make error handling explicit and predictable.
Currently, `getConfiguration` can already throw when an invalid properties
configuration is encountered. However, most other factories behave differently:
they either return `null` when the file is missing or the factory is inactive,
or return a partially constructed (broken) configuration when a parsing error
occurs.
> Unifying everything under the exception-throwing model would be
cleaner, but we should proceed carefully, as it could introduce
backward-compatibility issues for existing users.
Agreed with this approach. Since this is a new API, there are no backward
compatibility concerns for this particular method. In this PR we can throw on
configuration failures. In a follow-up PR, we can align the rest of the
`getConfiguration()` methods to do the same. Combining both tasks in a single
PR will not only violate the scope of this PR, but make it difficult to review.
@ppkarwasz, do you agree?
@yybmion, would you be interested in implementing changes necessary to make
other `ConfigurationFactory::getConfiguration` methods throw on failure too?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
