Add generic interface for loading service providers from plugins #88082
Conversation
grcevski
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| @@ -170,4 +170,7 @@ grant { | |||
|
|
|||
| // system memory on Linux systems affected by JDK bug (#66629) | |||
| permission java.io.FilePermission "/proc/meminfo", "read"; | |||
|
|
|||
| // needed for PluginsServiceTests | |||
| permission java.lang.RuntimePermission "createClassLoader"; | |||
There was a problem hiding this comment.
I've made a mistake here, wrong policy file. I'll submit a new commit.
| * For example: | ||
| * | ||
| * var pluginHandlers = pluginsService.loadServiceProviders(OperatorHandlerProvider.class); | ||
| * </p> |
There was a problem hiding this comment.
nit: I think the closing p tag is normally omitted in javadoc (or must be?)
There was a problem hiding this comment.
I thought it was allowed, I see we use it in few places, but it's stripped now and I'll remove it from other PR.
| * <p> | ||
| * For example: | ||
| * | ||
| * var pluginHandlers = pluginsService.loadServiceProviders(OperatorHandlerProvider.class); |
There was a problem hiding this comment.
This should be in a <pre> block?
| */ | ||
| public <T> List<? extends T> loadServiceProviders(Class<T> service) { | ||
| List<T> result = new ArrayList<>(); | ||
| getClass().getModule().addUses(service); |
There was a problem hiding this comment.
Why is this necessary? If a type is known in server, then it can be declared explicitly in server's module-info?
There was a problem hiding this comment.
I thought of it as a convenience, but we can always explicitly declare the use in module-info. I'll remove it.
|
|
||
| service = spy(service); | ||
|
|
||
| doReturn(List.of(provider1).iterator()).when(service).providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader)); |
There was a problem hiding this comment.
Since we are dealing with different classloaders, we should probably test more real-world situations. Can the test use real classloaders instead of mocking them? We have utilities for creating dummy jars with module infos, classes, etc. It seems doable to actually test the service loading.
There was a problem hiding this comment.
I brought back the real classloader test version.
There was a problem hiding this comment.
I still see mocking? To clarify what I was suggesting: dynamically build a dummy plugin jar and zip (with the utilities I mentioned) so the providersIterator method can actually be tested with the real ServiceLoader
There was a problem hiding this comment.
Ah I see, okay, I'll go further in building dummy plugins. This way I think I probably won't need to add extra permissions for the test framework.
|
@elasticmachine update branch |
| * <pre> | ||
| * For example: | ||
| * | ||
| * var pluginHandlers = pluginsService.loadServiceProviders(OperatorHandlerProvider.class); |
There was a problem hiding this comment.
Nit: I think only the code should be in the <pre>?
|
|
||
| service = spy(service); | ||
|
|
||
| doReturn(List.of(provider1).iterator()).when(service).providersIterator(eq(OperatorHandlerProvider.class), eq(fakeClassLoader)); |
There was a problem hiding this comment.
I still see mocking? To clarify what I was suggesting: dynamically build a dummy plugin jar and zip (with the utilities I mentioned) so the providersIterator method can actually be tested with the real ServiceLoader
|
Thanks for the review @rjernst, I removed all mocking, real SPI loading now. |
| @@ -293,6 +294,33 @@ static void loadExtensions(Collection<LoadedPlugin> plugins) { | |||
| } | |||
| } | |||
|
|
|||
| // package private for testing | |||
| <T> Iterator<T> providersIterator(Class<T> service, ClassLoader classLoader) { | |||
There was a problem hiding this comment.
I think this can be folded into loadServiceProviders now that the tests don't mock it?
| import org.elasticsearch.plugins.spi.TestService; | ||
| import org.elasticsearch.plugins.ActionPlugin; | ||
| import org.elasticsearch.plugins.Plugin; | ||
| public final class FooPlugin extends Plugin implements ActionPlugin, TestService { |
There was a problem hiding this comment.
I would separate this out. Due to the current plugin design, a Plugin subclass must be present, but this class should not be required to be the one that implements all service providers. In fact, I think it is important we test them decoupled, since that is the future we are working towards.
There was a problem hiding this comment.
Sure thing, I was just lazy :). I'll create a separate TestService provider class.
There was a problem hiding this comment.
Done now. I've split the provider class into separate file.
Since plugins/modules are loaded in their own class loader, loading service providers (SPI implementations) from various service interfaces in plugins is not a simple call to
ServiceLoader.load. This PR adds a helper method on PluginsService to load providers from plugins/modules given a service class.Example usage of this helper can be found here (in the related PR #86224):
https://github.com/elastic/elasticsearch/pull/86224/files#diff-b56bed42f5b0025886eb243ecb48678ac048bfba0ef047545dbb6504e357edf2R901
Relates to #86224