Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix custom catalog store plugin loading (v2) #22886

Merged
merged 4 commits into from
Aug 27, 2024
Merged

Fix custom catalog store plugin loading (v2) #22886

merged 4 commits into from
Aug 27, 2024

Commits on Aug 12, 2024

  1. Fix Guice bindings for CatalogStoreManager 

    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.
    @hashhar
    hashhar committed Aug 12, 2024
    Configuration menu
    Copy the full SHA
    0fd22cb View commit details
    Browse the repository at this point in the history

  2. Use catalog.store to choose CatalogStoreFactory 

    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
    hashhar committed Aug 12, 2024
    Configuration menu
    Copy the full SHA
    9bbcd7c View commit details
    Browse the repository at this point in the history

  3. Rename test class to match convention

    @hashhar
    hashhar committed Aug 12, 2024
    Configuration menu
    Copy the full SHA
    4dfc8ac View commit details
    Browse the repository at this point in the history

  4. Allow using custom catalog store with TestingTrinoServer

    @hashhar
    hashhar committed Aug 12, 2024
    Configuration menu
    Copy the full SHA
    2735a15 View commit details
    Browse the repository at this point in the history