JWT007 commented on issue #3458:
URL:
https://github.com/apache/logging-log4j2/issues/3458#issuecomment-2652350102
@ppkarwasz / @rgoers - I am ready to create a PR for this, but I noticed
something else - if you agree I will adjust the description and include the
change in the PR.
----
In the current `CompositeConfiguration#reconfigure()` it currently relies on
directly trying to reinstantiate the child configurations via the
`ConfigurationSource` URI.
```
@Override
public Configuration reconfigure() {
LOGGER.debug("Reconfiguring composite configuration");
final List<AbstractConfiguration> configs = new ArrayList<>();
final ConfigurationFactory factory = ConfigurationFactory.getInstance();
for (final AbstractConfiguration config : configurations) {
final ConfigurationSource source = config.getConfigurationSource();
final URI sourceURI = source.getURI();
Configuration currentConfig = config;
if (sourceURI == null) {
LOGGER.warn(
"Unable to determine URI for configuration {}, changes
to it will be ignored",
config.getName());
} else {
currentConfig = factory.getConfiguration(getLoggerContext(),
config.getName(), sourceURI);
if (currentConfig == null) {
LOGGER.warn("Unable to reload configuration {}, changes to
it will be ignored", config.getName());
}
}
configs.add((AbstractConfiguration) currentConfig);
}
return new CompositeConfiguration(configs);
}
```
This will not work for custom Configurations that cannot be instantiated
from the `ConfigurationFactory`(i.e. not the defaults from Log4j / custom
extensions).
I was wondering if taking advantage of the `Reconfigurable` interface might
be a more consistent approach for the `reconfigure()` design?
```
@Override
public Configuration reconfigure() {
LOGGER.debug("Reconfiguring composite configuration");
final List<AbstractConfiguration> configs = new ArrayList<>();
final ConfigurationFactory factory = ConfigurationFactory.getInstance();
for (final AbstractConfiguration config : configurations) {
Configuration currentConfig = config;
if (currentConfig instanceof Reconfigurable) {
try {
Configuration reloadedConfiguration = ((Reconfigurable)
currentConfig).reconfigure();
if (reloadedConfiguration != null) {
currentConfig = reloadedConfiguration;
} else {
LOGGER.warn("Unable to reload configuration {}, changes
to it will be ignored", config.getName());
continue; // continue loop so that null configuration
doesn't get added
}
} catch (final Exception ex) {
LOGGER.error("Unable to reconfigure configuration {},
changes to it will be ignored.",
config.getName(), ex);
}
}
configs.add((AbstractConfiguration) currentConfig);
}
return new CompositeConfiguration(configs);
}
```
One more thing - the original and my suggestion above both drop the original
configuration from the new Configuration if the reconfigure fails - this
results in the configurattion (and its configuration source) being lost.
For example if at runtime an XML configuration file is unavailable or locked
for some reason, it will fail the reconfigure and be removed from the composite
- a future reconfigure will not attempt to recover. _Should_ the original
configuration be kept as-is in this case?
IMHO it would generally be better if the 'Reconfigurable' interface never
expected null to be returned - for example the XmlConfigurattion returns an
empty configuration that keeps its ConfigurationSource instead of *null* - then
it can be added to the composite and attempted again in the next reconfigure.
Please give me a short feedback so I can finish up and submit a PR. :)
Thanks!
@rgoers (I included you because I *think* the original code is from you?)
--
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
