Support alternative extension implementations by kkondaka · Pull Request #5816 · opensearch-project/data-prepper

kkondaka · 2025-06-24T22:06:15Z

Description

Support alternative extension implementations. This allows multiple implementations for the same extension class with the extension with config provided is preferred over extension(s) without config provided. If multiple extensions with config are provided, the behavior is same as today, which is, it depends on the order in which the extensions are loaded. In future we will have a way to handle this.

Issues Resolved

Resolves #5792

Check List

New functionality includes testing.
New functionality has a documentation issue. Please link to it in this PR.
- New functionality has javadoc added
[X ] Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

dlvenable · 2025-06-24T23:08:02Z

                        return null;
                    }
                })
+                .sorted(Comparator.comparing(ExtensionPluginWithContext::isConfigured).reversed())


It would be ideal to have this Comparator in another class so that it can grow over time without having to change this method. And it could have it's own unit tests.

You could create a named bean in PluginCreatorContext.

@Bean(name = "extensionsLoaderComparator") public Comparator<ExtensionLoader.ExtensionPluginWithContext> extensionsLoaderComparator() { return Comparator.comparing(ExtensionPluginWithContext::isConfigured).reversed(); }

dlvenable · 2025-06-24T23:09:18Z

    private static final ExtensionProvider.Context EMPTY_CONTEXT = new EmptyContext();
    private final GenericApplicationContext sharedApplicationContext;
    private final GenericApplicationContext coreApplicationContext;
+    private Map<Class, Optional<Object>> providers;


I think a Set<Class<? extends ExtensionProvider>> should be sufficient. You don't need the value and you can avoid the call to extensionProvider.provideInstance(EMPTY_CONTEXT).

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

dlvenable · 2025-06-26T22:06:00Z

        this.pluginErrorsHandler = pluginErrorsHandler;
    }

+    ExtensionLoader(


Let's only have one constructor.

You can add this to the constructor above:

@Named("extensionsLoaderComparator" final Comparator<ExtensionLoader.ExtensionPluginWithContext> extensionsLoaderComparator

dlvenable · 2025-06-26T22:07:06Z

+
+        List<? extends ExtensionPlugin> result = null;
+        if (extensionPluginsWithContext != null && extensionPluginsWithContext.size() > 1 &&
+                extensionsLoaderComparator != null) {


I think we don't want this null check. This sorter always needs to be there. We could even add this to the constructor:

this.extensionsLoaderComparator = Objects.requireNonNull(extensionsLoaderComparator);

I think we can just drop the entire conditional.

dlvenable · 2025-06-26T22:07:56Z

        return new PluginCreator(pluginConfigurationObservableRegister);
    }
+
+    @Bean(name = "extensionsLoaderComparator")


You can add a simple unit test for this in PluginCreatorContextTest (add it if doesn't exist).

dlvenable · 2025-06-26T22:08:07Z

+        final String expectedPluginName = "test_extension_with_config";
+        when(extensionPluginConfigurationConverter.convert(eq(true), eq(TestExtensionConfig.class),
+                anyString())).thenReturn(new TestExtensionConfig());
+        //when(extensionPluginConfigurationConverter.convert(eq(true), eq(TestExtension.class),


Removed commented out code.

dlvenable · 2025-06-26T22:09:01Z

    @BeforeEach
    void setUp() {
        pluginErrorCollector = new PluginErrorCollector();
+        pluginCreatorContext = new PluginCreatorContext();


You could use a simple Comparator that sorts by name. That way these tests are not coupled with the sort logic.

Yes, but this way we are testing the actual sorting logic. I think this is better.

That is fine. But, we should have tests for the sort specifically. As the sort logic changes over time it will be much easier to isolate to just the Comparator test.

dlvenable · 2025-06-26T22:09:15Z


+    private PluginCreatorContext pluginCreatorContext;
+
+    @Mock


You are not using this as a mock, so you don't need this annotation.

dlvenable · 2025-06-26T22:09:45Z

    private ArgumentCaptor<Collection<PluginError>> pluginErrorsArgumentCaptor;
    private PluginErrorCollector pluginErrorCollector;

+    private PluginCreatorContext pluginCreatorContext;


You don't need this field. You can move it to a local variable in setUp. Also, you might not need it if you use a different Comparator.

I think I will keep the same comparator.

dlvenable · 2025-06-26T22:16:03Z

@kkondaka , Once you address these comments, this should be good to go. I think this is a solid change to extensions.

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

dlvenable · 2025-06-26T22:52:08Z

+    @Test
+    public void test_extensionsLoaderComparator() {
+        Comparator<ExtensionLoader.ExtensionPluginWithContext> extensionsLoaderComparator = pluginCreatorContext.extensionsLoaderComparator();
+        assertNotNull(extensionsLoaderComparator);


We should include tests that verify the sorting here. That way we can verify different sort combinations.

You don't have to test using sorts per se.

It could be as simple as these some of these combinations on compareTo:

Two objects both with isConfigured==true should have compareTo==0

Two objects both with isConfigured==false should have compareTo==0

Two objects, one with isConfigured==false and isConfigured==true, compareTo < 0 when true is on the left

Two objects, one with isConfigured==false and isConfigured==true, compareTo > 0 when true is on the right

It would be better to put these as different @Test cases, but I'm ok with it.

dlvenable · 2025-06-26T22:54:16Z

    @BeforeEach
    void setUp() {
        pluginErrorCollector = new PluginErrorCollector();
+        pluginCreatorContext = new PluginCreatorContext();


That is fine. But, we should have tests for the sort specifically. As the sort logic changes over time it will be much easier to isolate to just the Comparator test.

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

dlvenable · 2025-06-26T23:34:09Z

+    @Test
+    public void test_extensionsLoaderComparator() {
+        Comparator<ExtensionLoader.ExtensionPluginWithContext> extensionsLoaderComparator = pluginCreatorContext.extensionsLoaderComparator();
+        assertNotNull(extensionsLoaderComparator);


It would be better to put these as different @Test cases, but I'm ok with it.

* Support alternative extension implementations Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Addressed review comments and added tests Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Addressed review comments and added tests Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Addressed review comments Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> * Added tests Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> --------- Signed-off-by: Krishna Kondaka <krishkdk@amazon.com> Signed-off-by: Jonah Calvo <caljonah@amazon.com>

Support alternative extension implementations

e1412a2

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

dlvenable reviewed Jun 24, 2025

View reviewed changes

Addressed review comments and added tests

c867f5a

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

kkondaka marked this pull request as ready for review June 25, 2025 23:58

kkondaka requested review from KarstenSchnitter, chenqi0805, dinujoh, engechas, graytaylor0, oeyh, san81, sb2k16 and srikanthjg as code owners June 25, 2025 23:58

dlvenable reviewed Jun 26, 2025

View reviewed changes

Addressed review comments and added tests

98a4c60

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

dlvenable reviewed Jun 26, 2025

View reviewed changes

kkondaka added 2 commits June 26, 2025 23:06

Addressed review comments

8270b90

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

Added tests

d9ccc4b

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

dlvenable approved these changes Jun 26, 2025

View reviewed changes

dinujoh approved these changes Jun 28, 2025

View reviewed changes

sb2k16 approved these changes Jun 30, 2025

View reviewed changes

kkondaka merged commit 9409ba7 into opensearch-project:main Jun 30, 2025
47 of 51 checks passed

kkondaka deleted the extensions-provider branch July 1, 2025 17:04

dlvenable mentioned this pull request Aug 22, 2025

Allow extensions to depend upon other extensions #2825

Closed

2 tasks

Conversation

kkondaka commented Jun 24, 2025

Description

Issues Resolved

Check List

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlvenable commented Jun 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants