From 0fd22cb8a9408309482fb75c98d3293e0508a898 Mon Sep 17 00:00:00 2001 From: Ashhar Hasan Date: Mon, 12 Aug 2024 17:43:32 +0530 Subject: [PATCH 1/4] 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. --- .../java/io/trino/connector/DynamicCatalogManagerModule.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/core/trino-main/src/main/java/io/trino/connector/DynamicCatalogManagerModule.java b/core/trino-main/src/main/java/io/trino/connector/DynamicCatalogManagerModule.java index 97ad4619469e2..7ba6bef1beb3a 100644 --- a/core/trino-main/src/main/java/io/trino/connector/DynamicCatalogManagerModule.java +++ b/core/trino-main/src/main/java/io/trino/connector/DynamicCatalogManagerModule.java @@ -24,7 +24,6 @@ import java.util.Locale; -import static com.google.inject.multibindings.OptionalBinder.newOptionalBinder; import static io.airlift.configuration.ConfigBinder.configBinder; public class DynamicCatalogManagerModule @@ -44,7 +43,6 @@ protected void setup(Binder binder) } default -> { binder.bind(CatalogStoreManager.class).in(Scopes.SINGLETON); - newOptionalBinder(binder, CatalogStoreManager.class).setBinding().to(CatalogStoreManager.class).in(Scopes.SINGLETON); binder.bind(CatalogStore.class).to(CatalogStoreManager.class).in(Scopes.SINGLETON); } } From 9bbcd7c3e371647ed24c2491f7539f9b5a4646d2 Mon Sep 17 00:00:00 2001 From: Ashhar Hasan Date: Mon, 12 Aug 2024 17:47:11 +0530 Subject: [PATCH 2/4] 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. --- .../trino/connector/CatalogStoreManager.java | 23 ++++++++----------- .../connector/TestCatalogStoreManager.java | 17 +++++++++++--- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/core/trino-main/src/main/java/io/trino/connector/CatalogStoreManager.java b/core/trino-main/src/main/java/io/trino/connector/CatalogStoreManager.java index a31b953d97298..96fd94b00b27a 100644 --- a/core/trino-main/src/main/java/io/trino/connector/CatalogStoreManager.java +++ b/core/trino-main/src/main/java/io/trino/connector/CatalogStoreManager.java @@ -34,9 +34,7 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicReference; -import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; -import static com.google.common.base.Strings.isNullOrEmpty; import static io.airlift.configuration.ConfigurationLoader.loadPropertiesFrom; import static java.util.Objects.requireNonNull; @@ -45,15 +43,16 @@ public class CatalogStoreManager { private static final Logger log = Logger.get(CatalogStoreManager.class); private static final File CATALOG_STORE_CONFIGURATION = new File("etc/catalog-store.properties"); - private static final String CATALOG_STORE_PROPERTY_NAME = "catalog-store.name"; private final Map catalogStoreFactories = new ConcurrentHashMap<>(); private final AtomicReference> configuredCatalogStore = new AtomicReference<>(Optional.empty()); private final SecretsResolver secretsResolver; + private final String catalogStoreKind; @Inject - public CatalogStoreManager(SecretsResolver secretsResolver) + public CatalogStoreManager(SecretsResolver secretsResolver, CatalogStoreConfig catalogStoreConfig) { this.secretsResolver = requireNonNull(secretsResolver, "secretsResolver is null"); + this.catalogStoreKind = requireNonNull(catalogStoreConfig.getCatalogStoreKind(), "catalogStoreKind is null"); } public void addCatalogStoreFactory(CatalogStoreFactory catalogStoreFactory) @@ -68,22 +67,20 @@ public void addCatalogStoreFactory(CatalogStoreFactory catalogStoreFactory) public void loadConfiguredCatalogStore() throws IOException { - loadConfiguredCatalogStore(CATALOG_STORE_CONFIGURATION); + loadConfiguredCatalogStore(catalogStoreKind, CATALOG_STORE_CONFIGURATION); } @VisibleForTesting - void loadConfiguredCatalogStore(File catalogStoreFile) + void loadConfiguredCatalogStore(String catalogStoreName, File catalogStoreFile) throws IOException { - if (configuredCatalogStore.get().isPresent() || !catalogStoreFile.exists()) { + if (configuredCatalogStore.get().isPresent()) { return; } - Map properties = new HashMap<>(loadPropertiesFrom(catalogStoreFile.getPath())); - - String catalogStoreName = properties.remove(CATALOG_STORE_PROPERTY_NAME); - checkArgument(!isNullOrEmpty(catalogStoreName), - "Catalog store configuration %s does not contain %s", catalogStoreFile.getAbsoluteFile(), CATALOG_STORE_PROPERTY_NAME); - + Map properties = new HashMap<>(); + if (catalogStoreFile.exists()) { + properties = new HashMap<>(loadPropertiesFrom(catalogStoreFile.getPath())); + } setConfiguredCatalogStore(catalogStoreName, properties); } diff --git a/core/trino-main/src/test/java/io/trino/connector/TestCatalogStoreManager.java b/core/trino-main/src/test/java/io/trino/connector/TestCatalogStoreManager.java index 747244355e403..8df714b498417 100644 --- a/core/trino-main/src/test/java/io/trino/connector/TestCatalogStoreManager.java +++ b/core/trino-main/src/test/java/io/trino/connector/TestCatalogStoreManager.java @@ -38,14 +38,25 @@ void testCatalogStoreIsLoaded() throws IOException { try (TempFile tempFile = new TempFile()) { - Files.writeString(tempFile.path(), "catalog-store.name=test"); - CatalogStoreManager catalogStoreManager = new CatalogStoreManager(new SecretsResolver(ImmutableMap.of())); + Files.writeString(tempFile.path(), "some-property=some-value"); + CatalogStoreConfig catalogStoreConfig = new CatalogStoreConfig().setCatalogStoreKind("test"); + CatalogStoreManager catalogStoreManager = new CatalogStoreManager(new SecretsResolver(ImmutableMap.of()), catalogStoreConfig); catalogStoreManager.addCatalogStoreFactory(new TestingCatalogStoreFactory()); - catalogStoreManager.loadConfiguredCatalogStore(tempFile.file()); + catalogStoreManager.loadConfiguredCatalogStore(catalogStoreConfig.getCatalogStoreKind(), tempFile.file()); assertThat(catalogStoreManager.getCatalogs()).containsExactly(TestingCatalogStore.STORED_CATALOG); } } + @Test + void testCatalogStoreIsLoadedWithoutConfiguration() + throws IOException + { + CatalogStoreManager catalogStoreManager = new CatalogStoreManager(new SecretsResolver(ImmutableMap.of()), new CatalogStoreConfig().setCatalogStoreKind("test")); + catalogStoreManager.addCatalogStoreFactory(new TestingCatalogStoreFactory()); + catalogStoreManager.loadConfiguredCatalogStore(); + assertThat(catalogStoreManager.getCatalogs()).containsExactly(TestingCatalogStore.STORED_CATALOG); + } + private static class TestingCatalogStoreFactory implements CatalogStoreFactory { From 4dfc8ac24e760f1fe7e1d5754262f1dda032c259 Mon Sep 17 00:00:00 2001 From: Ashhar Hasan Date: Wed, 24 Jul 2024 14:10:48 +0530 Subject: [PATCH 3/4] Rename test class to match convention --- ...stingCatalogStoreConfig.java => TestCatalogStoreConfig.java} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename core/trino-main/src/test/java/io/trino/connector/{TestingCatalogStoreConfig.java => TestCatalogStoreConfig.java} (97%) diff --git a/core/trino-main/src/test/java/io/trino/connector/TestingCatalogStoreConfig.java b/core/trino-main/src/test/java/io/trino/connector/TestCatalogStoreConfig.java similarity index 97% rename from core/trino-main/src/test/java/io/trino/connector/TestingCatalogStoreConfig.java rename to core/trino-main/src/test/java/io/trino/connector/TestCatalogStoreConfig.java index 910a02c54448c..da8e28d9349ad 100644 --- a/core/trino-main/src/test/java/io/trino/connector/TestingCatalogStoreConfig.java +++ b/core/trino-main/src/test/java/io/trino/connector/TestCatalogStoreConfig.java @@ -22,7 +22,7 @@ import static io.airlift.configuration.testing.ConfigAssertions.assertRecordedDefaults; import static io.airlift.configuration.testing.ConfigAssertions.recordDefaults; -public class TestingCatalogStoreConfig +public class TestCatalogStoreConfig { @Test public void testDefaults() From 2735a151b8f0d09962e6736225a8f9415f3a129e Mon Sep 17 00:00:00 2001 From: Ashhar Hasan Date: Wed, 31 Jul 2024 13:32:46 +0530 Subject: [PATCH 4/4] Allow using custom catalog store with TestingTrinoServer --- .../server/testing/TestingTrinoServer.java | 5 +++- .../testing/TestTestingTrinoServer.java | 23 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/core/trino-main/src/main/java/io/trino/server/testing/TestingTrinoServer.java b/core/trino-main/src/main/java/io/trino/server/testing/TestingTrinoServer.java index 5f87c6825aae3..fafede4cf027b 100644 --- a/core/trino-main/src/main/java/io/trino/server/testing/TestingTrinoServer.java +++ b/core/trino-main/src/main/java/io/trino/server/testing/TestingTrinoServer.java @@ -276,7 +276,10 @@ private TestingTrinoServer( if (coordinator) { if (catalogMangerKind == CatalogMangerKind.DYNAMIC) { - serverProperties.put("catalog.store", "memory"); + Optional catalogStore = Optional.ofNullable(properties.get("catalog.store")); + if (catalogStore.isEmpty()) { + serverProperties.put("catalog.store", "memory"); + } } serverProperties.put("failure-detector.enabled", "false"); diff --git a/core/trino-main/src/test/java/io/trino/server/testing/TestTestingTrinoServer.java b/core/trino-main/src/test/java/io/trino/server/testing/TestTestingTrinoServer.java index 978cc6ddd1c24..acfcfacd17e04 100644 --- a/core/trino-main/src/test/java/io/trino/server/testing/TestTestingTrinoServer.java +++ b/core/trino-main/src/test/java/io/trino/server/testing/TestTestingTrinoServer.java @@ -16,6 +16,7 @@ import com.google.inject.Key; import io.trino.connector.ConnectorServicesProvider; import io.trino.connector.CoordinatorDynamicCatalogManager; +import io.trino.connector.FileCatalogStore; import io.trino.connector.InMemoryCatalogStore; import io.trino.connector.StaticCatalogManager; import io.trino.connector.WorkerDynamicCatalogManager; @@ -63,4 +64,26 @@ void testSetCatalogManagementToStatic() .isInstanceOf(StaticCatalogManager.class); } } + + @Test + void testDefaultCatalogStore() + throws IOException + { + try (TestingTrinoServer server = TestingTrinoServer.builder().build()) { + assertThat(server.getInstance(Key.get(CatalogStore.class))) + .isInstanceOf(InMemoryCatalogStore.class); + } + } + + @Test + void testExplicitCatalogStore() + throws IOException + { + try (TestingTrinoServer server = TestingTrinoServer.builder() + .addProperty("catalog.store", "file") + .build()) { + assertThat(server.getInstance(Key.get(CatalogStore.class))) + .isInstanceOf(FileCatalogStore.class); + } + } }