Skip to content

Introduce PluginInstaller#16060

Merged
kokosing merged 1 commit intotrinodb:masterfrom
skrzypo987:skrzypo/265-plugin-manager-extension
Feb 14, 2023
Merged

Introduce PluginInstaller#16060
kokosing merged 1 commit intotrinodb:masterfrom
skrzypo987:skrzypo/265-plugin-manager-extension

Conversation

@skrzypo987
Copy link
Copy Markdown
Member

Description

Additional context and related issues

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@skrzypo987 skrzypo987 changed the title Support plugin installation via custom handlers Introduce PluginInstaller Feb 10, 2023
@martint
Copy link
Copy Markdown
Member

martint commented Feb 10, 2023

What would a such a plugin do if Trino is not aware of it and doesn’t know how to use it?

Please describe the motivation for this change.

@kokosing
Copy link
Copy Markdown
Member

What would a such a plugin do if Trino is not aware of it and doesn’t know how to use it?

@martint One can use io.trino.server.Server#getAdditionalModules to inject support for custom functionality.

Also please notice #16066 that also presents the value of PluginInstaller to Trino itself. Thanks to this PluginManager follows open-close principle whenever a new type of the plugin is added.

@martint
Copy link
Copy Markdown
Member

martint commented Feb 10, 2023

But a plugin needs to be inserted somewhere in the flow of a query. How would plugins added via a PluginInstaller be injected into the flow if the engine has no idea what the plugin is for?

@kokosing
Copy link
Copy Markdown
Member

But a plugin needs to be inserted somewhere in the flow of a query.

Yes, for example SystemSecurityMetadata can be customized as we are using optional binder. Then custom plugin may affect it is behavior.

However, my motivation was to allow plugin to affect behavior of other processes than query execution that were added to customized Trino. IMO allowing changes of query behavior should be only allowed only with dedicated SPI. Query execution is too complex to be reasonably customized.

That way when adding new Plugin type we can provide associated with
plugin installer that we handle it installation without changes in PluginManager.
@skrzypo987 skrzypo987 force-pushed the skrzypo/265-plugin-manager-extension branch from c2384d8 to 71b40a3 Compare February 13, 2023 07:30
@skrzypo987 skrzypo987 requested a review from kokosing February 14, 2023 07:03
@kokosing kokosing merged commit 6c05ae1 into trinodb:master Feb 14, 2023
@github-actions github-actions bot added this to the 407 milestone Feb 14, 2023
@martint
Copy link
Copy Markdown
Member

martint commented Feb 14, 2023

However, my motivation was to allow plugin to affect behavior of other processes than query execution that were added to customized Trino.

You don’t need a plugin for that. You can just bind additional Guice modules to extend the Trino server if you need additional functionality.

It’s still not clear to me what problem we’re trying to solve.

@kokosing
Copy link
Copy Markdown
Member

You don’t need a plugin for that. You can just bind additional Guice modules to extend the Trino server if you need additional functionality.

Yes. But if you want to have this functionality in the plugin, behind the plugin classloader isolation and as part of the bigger plugin then additional plugins are not enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants