logging-log4j2
logging-log4j2 copied to clipboard
apache
Load pre-loaded lookups via SPI, rather than hard-code in Interpolator.
What's wrong with the CI? The change should have nothing to do with the error. :)
No worries about the Travis check. The GitHub Actions checks are the important ones. We need to disable our old Travis checks.
The only concern I have with this is the overhead of ServiceLoader during startup. I recently had to make ServiceLoader run asynchronously during startup while loading Custom ThreadContextDataProviders. This will likely cause the same problem. We should make that more generic so that components such as them can register their service for asynchronous initialization. But that would be done in a different PR.
@jvz I dislike ServiceLookup as it is describing its implementation method, not its purpose, which is to allow a way for users to hook into a Lookup that is configured to run before any standard plugins have been processed.
Perhaps that is an indication that the mechanism provided here is incorrect. Perhaps the service loader should be looking for Lookup Plugins that are also declared as services. We could replace the way all these lookups are loaded if we do that.
@rgoers Agree! This PR is intended to do that as you proposed. Hope this could be supported ASAP :) . Have a nice day.
@rgoers Do you mean all the pre-loaded Lookups should be loaded via ServiceLoader? I am glad to contribute to this part if you do not mind. :)
Yes, that is exactly what I mean. Feel free to modify this PR to do that. In essence, StrLookup would become the service interface and every plugin to be pre-loaded would be listed in the services file instead of hard coded in Interpolator.
@jvz @rgoers It seems the Log4jPlugins class is moved from org.apache.logging.log4j.core.plugins. However in Activator class, Log4jPlugins is still imported as "org.apache.logging.log4j.core.plugins.Log4jPlugins".
@rgoers @jvz Hi, are you busy? Could you get round to take a review and merge :).
These changes are not what I expected. All the Lookups should remain plugins. You should have only had to modify Interpolator to load the required lookups via ServiceLoader and then added the Lookups you wanted to load to the file used by ServiceLoader. The Lookups would be loaded again as normal as Plugins during configuration.
@rgoers I propably misunderstood your opinion:
In essence, StrLookup would become the service interface and every plugin to be pre-loaded would be listed in the services file instead of hard coded in Interpolator.
I guess following is what you want:
- Load all lookups as plugins via SPI.
- Load all lookup plugins via the plugin mechanism.
Is it right ?
@rgoers Hi, rgoers. After debuging and reading the log4j2 codes, I realize that the public Interpolator(final Map<String, String> properties) is called several times during factory creation stage. Is that reasonable to create so many StrLookup instances with the same type?
Well, the Interpolator constructor that has the hard-coded list of lookups is initializing the lookups that could be using while constructing the configuration. The other lookups would be used at runtime. There is no reason that I can think of that all the Lookups couldn't be initialized once via ServiceLoader except that it would break compatibility with users who have created their own Lookups as plugins. We could possibly load the serviceloader Lookups once and then add the ones defined as plugins later.
This was closed automatically by Github because we renamed the release-2.x branch to 2.x. Feel free to resubmit to the 2.x branch.
