Initial support for plugin configuration classes #478
Conversation
Signed-off-by: David Venable <[email protected]>
|
I updated the |
| @@ -48,7 +52,9 @@ public DefaultPluginFactory() { | |||
| final String pluginName = pluginSetting.getName(); | |||
| final Class<? extends T> pluginClass = getPluginClass(baseClass, pluginName); | |||
|
|
|||
| return pluginCreator.newPluginInstance(pluginClass, pluginSetting); | |||
| final Object configuration = getConfiguration(pluginSetting, pluginClass); | |||
There was a problem hiding this comment.
Really looking forward to getting rid of Object as a type that holds configuration...
There was a problem hiding this comment.
I believe in this case we need to stick with Object. The type is determined at runtime with this change. There might be an approach to making this work with generics, but I suspect it would be overly complicated without providing any value.
There was a problem hiding this comment.
Sad 😢 , we can leave it as is for right now, but perhaps the work that Shivani is doing with the configuration models can help this.
There was a problem hiding this comment.
We could move away from Object by using inheritence and providing a base class with common configuration attirbutes like the plugin metrics. But I think you were thinking of keeping these objects separate have your considered combining?
| if(pluginConfigurationType.equals(PluginSetting.class)) | ||
| return pluginSetting; | ||
|
|
||
| return objectMapper.convertValue(pluginSetting.getSettings(), pluginConfigurationType); |
There was a problem hiding this comment.
A nit on my part, but I'd like to see this as an if / else block.
| */ | ||
| public StringPrepper(final PluginSetting pluginSetting) { | ||
| this.upperCase = pluginSetting.getBooleanOrDefault(UPPER_CASE, true); | ||
| public StringPrepper(final Configuration configuration) { |
There was a problem hiding this comment.
@laneholloway , There is where the Object referred to in another discussion ends up being used. The plugin code gets a nice type and does not need to cast. I believe that this is very valuable for plugin authors and wanted to point it out.
There was a problem hiding this comment.
Nice, thanks for showing how a plugin may use the new configuration model
There was a problem hiding this comment.
My only other comment would be documenting this approach so other's can leverage the new model. I thought @laneholloway had put together a guide for developing plugins but I can't seem to find it. Perhaps he is still working on it.
| @@ -48,7 +52,9 @@ public DefaultPluginFactory() { | |||
| final String pluginName = pluginSetting.getName(); | |||
| final Class<? extends T> pluginClass = getPluginClass(baseClass, pluginName); | |||
|
|
|||
| return pluginCreator.newPluginInstance(pluginClass, pluginSetting); | |||
| final Object configuration = getConfiguration(pluginSetting, pluginClass); | |||
There was a problem hiding this comment.
We could move away from Object by using inheritence and providing a base class with common configuration attirbutes like the plugin metrics. But I think you were thinking of keeping these objects separate have your considered combining?
| */ | ||
| public StringPrepper(final PluginSetting pluginSetting) { | ||
| this.upperCase = pluginSetting.getBooleanOrDefault(UPPER_CASE, true); | ||
| public StringPrepper(final Configuration configuration) { |
There was a problem hiding this comment.
Nice, thanks for showing how a plugin may use the new configuration model
|
Regarding documentation, Data Prepper doesn't currently have a plugin development guide. At least part of the problem is that it doesn't yet support plugins from external sources. I'll look into adding more documentation after making all the other upcoming changes. For the |
Description
This PR includes code which provides a mechanism for converting from
PluginSettinginto a custom class as defined by the plugin in@DataPrepperPlugin. This is the first PR for some of the work proposed in #469 for straightforward use-cases.Issues Resolved
Provides some functionality for #469.
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.