Add option to provide URIs to monitor in addition to the config file

[MichaelMorrisEst]

A change detected in either the config file itself or the provided additional URIs shall result in a reconfigure
See #3074

Checklist

• Base your changes on 2.x branch if you are targeting Log4j 2; use main otherwise
• ./mvnw verify succeeds (if it fails due to code formatting issues reported by Spotless, simply run ./mvnw spotless:apply and retry)
• Non-trivial changes contain an entry file in the src/changelog/.2.x.x directory
• Tests for the changes are provided
• Commits are signed (optional, but highly recommended)

[vy]

@MichaelMorrisEst, thanks so much for putting effort into this, much appreciated! 😍

I've dropped some remarks. I'd appreciate it if you can update the PR with requested changes. I will have some more remarks – e.g., adding docs – but I will share them last.

[MichaelMorrisEst]

Thanks for your comments @vy. The comment to use a dedicated element instead of an attribute changes the implementation quiet a bit. Please take a look when you have time, feedback gratefully received. Thanks!

[vy]

@MichaelMorrisEst, I am extremely thankful for your effort and patience. I've some questions. I'd appreciate it if you can address them.

[vy]

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.

[MichaelMorrisEst]

My thinking here was to be consistent with other error scenarios, for example lines 726, 764, 790.
Also to let the exception propagate we would need to declare it on the doConfigure() method which could cause an impact on anyone with their own custom implementation which extends AbstractConfiguration

[vy]

See this recent discussion on how to handle configuration failures. 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.

[github-actions]

Job	Requested goals	Build Tool Version	Build Outcome	Build Scan®
build-macos-latest	clean install	3.9.8	✅
build-ubuntu-latest	clean install	3.9.8	✅
build-windows-latest	clean install	3.9.8	✅

Generated by gradle/develocity-actions

[vy]

Would you mind also updating configuration.adoc such that

1. Just before Arbiters section, create a Monitor URIs section.
2. Link Monitor URIs and #configuration-attribute-monitorInterval to each other with sufficient text and explanation.

[vy]

Can't we simplify this as follows? (Can uris be null at all?)

Suggested change

	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());
	}
	LOGGER.info(
	"Start watching for changes to {} and {} every {} seconds",
	getConfigurationSource(),
	uris,
	watchManager.getIntervalSeconds());
	watchMonitorUris();

[vy]

... 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?

[MichaelMorrisEst]

Hi @vy
The reason I did not put it in initializeWatchers() is because initializeWatchers() is called before the elements are read. The top level attributes (including monitorInterval) have been read at this stage and can therefore be used, but the elements (including monitorUris) have not been.

Would it be ok to move invoking watchMonitorUris() to intialization() which is earlier than here, but after the elements have been read? This would avoid having to read the monitorUri element earlier and separately from the other elements.

I can move it to initializeWatchers() either if you still prefer that, and add code to read the monitorUri element separately to the other elements, but I think the above would be cleaner in the code.

[vy]

See this recent discussion on how to handle configuration failures. 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.