Add support for extensions to depend upon other extensions#6012
Conversation
graytaylor0
requested review from
KarstenSchnitter,
chenqi0805,
dinujoh,
dlvenable,
engechas,
kkondaka,
oeyh,
san81,
sb2k16 and
srikanthjg
as code owners
Signed-off-by: Taylor Gray <tylgry@amazon.com>
graytaylor0
force-pushed
the
ExtensionsChange
branch
from
cc7534b to
786b621
Compare
| * Data Prepper as an extension plugin. Everything starts from here. | ||
| */ | ||
| @DataPrepperExtensionPlugin(modelType = AwsPluginConfig.class, rootKeyJsonPath = "/aws/configurations") | ||
| @ExtensionProvides(providedClasses = {AwsCredentialsSupplier.class}) |
There was a problem hiding this comment.
Why is this needed? AwsCredentialsSupplier is not an extension class, right?
There was a problem hiding this comment.
AwsCredentialsSupplier is not an extension class. It is provided by an extension class. This is declaring that this particular extension provides it.
|
|
||
| @Override | ||
| public <T> T getExtensionProvider(final Class<T> type) { | ||
| sharedApplicationContext.refresh(); |
There was a problem hiding this comment.
We found that multiple calls to refresh() will fail. So this solution only works with one dependency. This is fine for now, but we need to remember to fix this.
| * Data Prepper as an extension plugin. Everything starts from here. | ||
| */ | ||
| @DataPrepperExtensionPlugin(modelType = AwsPluginConfig.class, rootKeyJsonPath = "/aws/configurations") | ||
| @ExtensionProvides(providedClasses = {AwsCredentialsSupplier.class}) |
There was a problem hiding this comment.
AwsCredentialsSupplier is not an extension class. It is provided by an extension class. This is declaring that this particular extension provides it.
| public class ExtensionPluginWithContext { | ||
| ExtensionPlugin extensionPlugin; | ||
| boolean configured; | ||
| Class<?>[] dependentClasses; |
There was a problem hiding this comment.
These can be final (along with the two above).
| return (extensionOne, extensionTwo) -> { | ||
| // First, compare by configuration status (configured ones first) | ||
| int configCompare = Boolean.compare(extensionTwo.isConfigured(), extensionOne.isConfigured()); | ||
| if (configCompare != 0) { |
There was a problem hiding this comment.
This is probably not the final sorting. It may be the case a configured depends on a non-configured. In which case the dependency will not exist. But, we can handle this later.
|
We may be able to improve the Spring issue and even let Spring do the ordering by making use of Spring's support to register beans using a If we move the The thing I'm unsure of is how to tell Spring of the dependency between the two. |
3 participants
Description
This change allows for extensions to depend on other extensions by loading them in the correct order based on the classes they provide and their dependencies.
The plugins will be loaded in the order by checking
Tested by loading aws secrets plugin with AwsCredentialsSupplier and verifying that the supplier is accessible
Issues Resolved
Resolves #2825
Check List
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.