Fix custom catalog store plugin loading (v2) #22886
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hashhar
changed the title
piotrrzysko
approved these changes
Jul 31, 2024
dain
reviewed
Jul 31, 2024
core/trino-main/src/main/java/io/trino/connector/CatalogStoreKind.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/connector/DynamicCatalogManagerModule.java
Outdated
Show resolved
Hide resolved
CatalogStoreManager is provided as an Optional (but empty) binding in
CatalogManagerModule since it's required by the PluginManager.
In DynamicCatalogManagerModule we provide two bindings for the
CatalogStoreManager against the same Guice key which is invalid and
leads to failures like:
3) [Guice/BindingAlreadySet]: CatalogStoreManager was bound multiple times.
Bound at:
1 : DynamicCatalogManagerModule.setup(DynamicCatalogManagerModule.java:47)
\_ installed by: CatalogManagerModule -> DynamicCatalogManagerModule
2 : java.base/DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
\_ installed by: CatalogManagerModule -> DynamicCatalogManagerModule
Learn more:
https://github.com/google/guice/wiki/BINDING_ALREADY_SET
Another issue is that the optional binder binds CatalogStoreManager to
itself which Guice also complains about:
1) [Guice/RecursiveBinding]: Binding points to itself. Key: CatalogStoreManager
at CatalogManagerModule.setup(CatalogManagerModule.java:31)
\_ installed by: CatalogManagerModule -> RealOptionalBinder
2) [Guice/RecursiveBinding]: Binding points to itself. Key: CatalogStoreManager
at DynamicCatalogManagerModule.setup(DynamicCatalogManagerModule.java:47)
\_ installed by: CatalogManagerModule -> DynamicCatalogManagerModule
This can be solved by introducing an interface for CatalogStoreManager
and binding against that but since we'll only ever have a single
implementation we don't need that. Instead Guice can satisfy optional
bindings using user-provided bindings too which allows us to simply
remove the setBinding on the optional binder.
From JavaDocs of `OptionalBinder`
(https://google.github.io/guice/api-docs/4.2/javadoc/com/google/inject/multibindings/OptionalBinder.html).
If neither setDefault nor setBinding are called, it will try to link to
a user-supplied binding of the same type. If no binding exists, the
optionals will be absent. Otherwise, if a user-supplied binding of that
type exists, or if setBinding or setDefault are called, the optionals
will return present if they are bound to a non-null value.
Before this change the CatalogStoreFactory which was chosen was read from `catalog-store.name` from `etc/catalog-store.properties`. Now we use the value of `catalog.store` to choose the correct catalog store implementation and remove the `catalog-store.name` property. This change additionally allows the `etc/catalog-store.properties` file to be missing in which case it passes an empty configuration to the `CatalogStoreFactory#create`. This is done because not all catalog store implementations require properties and forcing users to create an empty config file is both confusing and misleading.
hashhar
force-pushed
the
hashhar/catalog-store-fixes-one-more-time
branch
from
81d2290
to
2735a15
Compare
|
Addressed your comments Dain. PTAL. I'll send a separate PR that moves file + memory catalog store to be loaded using the |
|
Forgot to tag you @dain. 😄 |
sebadiaz
reviewed
Aug 13, 2024
| assertThat(server.getInstance(Key.get(CatalogStore.class))) | ||
| .isInstanceOf(FileCatalogStore.class); | ||
| } | ||
| } |
There was a problem hiding this comment.
You should test the Dynamic custom store.
In invoking a New plugin based on catalogstore catalogstorefactory and plugin interface that contains the getcatalogstorefactoriesn method.
My last test in your code configuration let me confuse. I m not sure that it works well on a custom plugin for catalog store.
There was a problem hiding this comment.
I've tested this with a custom catalog store (that always returns single catalog). I am submitting a follow up PR as mentioned in above comment which would allow me to add a test.
dain
approved these changes
Aug 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Currently a custom catalog store plugin cannot be used because the Guice bindings are incorrect.
Fixes #22554
Supercedes #22784
I couldn't think of any way to test this other than a product test - but that too would need me to add a custom plugin to the build just to test this - I manually tested this instead for now.
Additional context and related issues
See #22554 for details. And #20489 for earlier changes.
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: