-
Notifications
You must be signed in to change notification settings - Fork 202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH: support plugin loading in conifg #4974
ENH: support plugin loading in conifg #4974
Conversation
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
@@ -36,7 +47,7 @@ | |||
@JsonSerialize(using = PluginModel.PluginModelSerializer.class) | |||
@JsonDeserialize(using = PluginModel.PluginModelDeserializer.class) | |||
public class PluginModel { | |||
|
|||
static final String DEFAULT_PLUGINS_CLASSPATH = "org.opensearch.dataprepper.plugins"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this approach is the right one for a few reasons:
- This is leaking information on the plugins classpath out of core and into Data Prepper API.
- This class deals with basic serialization and this approach is coupling it with the plugin framework to some degree.
- This duplicates concerns and code from the plugin framework.
- The plugin framework supports loading plugins from other packages as well, and this does not honor that.
This class - PluginModel
- should remain concerned only with the most basic form of a plugin.
Use the plugin framework to detect this metadata. Then, have the schema converter use the plugin framework to get the metadata out of there. It may necessitate new method method on PluginFactory
(though I'm not sure on this yet).
.orElse(null)); | ||
} | ||
|
||
private void overrideDataPrepperPluginTypeAttribute( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scanning for plugins should only be done in the plugin framework. Make modifications there and use those.
* @return The Java class | ||
* @since 1.2 | ||
*/ | ||
Class<?> pluginType(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't need this if we support it on the target itself.
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
data-prepper-api/build.gradle
Outdated
@@ -12,9 +12,10 @@ dependencies { | |||
implementation 'com.fasterxml.jackson.core:jackson-databind' | |||
implementation 'com.fasterxml.jackson.datatype:jackson-datatype-jsr310' | |||
implementation 'com.fasterxml.jackson.datatype:jackson-datatype-jdk8' | |||
implementation libs.reflections.core |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need this anymore.
@@ -54,7 +54,7 @@ dependencyResolutionManagement { | |||
library('bouncycastle-bcpkix', 'org.bouncycastle', 'bcpkix-jdk18on').versionRef('bouncycastle') | |||
version('guava', '32.1.2-jre') | |||
library('guava-core', 'com.google.guava', 'guava').versionRef('guava') | |||
version('reflections', '0.9.12') | |||
version('reflections', '0.10.2') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this update!
Signed-off-by: George Chen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @chenqi0805 for this improvement!
--------- Signed-off-by: George Chen <[email protected]>
Description
This PR
anyOf
schemas for embedded pluginModel attribute to represent enum schemase.g. with the change in the PR, the aggregate processor schema will look like
Issues Resolved
Contributes toward #4838
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.