From 8d3eb2782de4070015473ead1ea8bc77640852bb Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Mon, 11 Oct 2021 10:22:32 +0200 Subject: [PATCH 1/2] Flatten Iceberg catalog/metastore configuration Hive metastore is not sufficient abstraction for Iceberg, since locking protocol will be different for different `HiveMetastore` implementations. Flattening configuration allows us to clearly define what catalogs are supported, and also reduces configuration complexity from end user perspective. This removes use of `hive.metastore` config from Iceberg in favor of already existing `iceberg.catalog.type`. --- .../io/trino/plugin/iceberg/CatalogType.java | 7 +++--- .../trino/plugin/iceberg/IcebergConfig.java | 4 ++-- .../iceberg/IcebergMetastoreModule.java | 13 +++++----- .../trino/plugin/iceberg/IcebergModule.java | 1 + .../plugin/iceberg/TrinoCatalogFactory.java | 6 +++-- .../plugin/iceberg/IcebergQueryRunner.java | 2 +- .../plugin/iceberg/TestIcebergConfig.java | 10 ++++---- .../plugin/iceberg/TestIcebergPlugin.java | 24 +++++++++++++++---- 8 files changed, 42 insertions(+), 25 deletions(-) diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/CatalogType.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/CatalogType.java index 8bf3aa63ca09..35f46922a785 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/CatalogType.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/CatalogType.java @@ -15,9 +15,8 @@ public enum CatalogType { - HIVE, - // TODO: dummy type to pass IcebergConfig test, remove it after adding actual catalog types - UNKNOWN, - + TESTING_FILE_METASTORE, + HIVE_METASTORE, + GLUE, /**/; } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java index c538fe644578..079cb3595487 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java @@ -22,7 +22,7 @@ import javax.validation.constraints.NotNull; import static io.trino.plugin.hive.HiveCompressionCodec.GZIP; -import static io.trino.plugin.iceberg.CatalogType.HIVE; +import static io.trino.plugin.iceberg.CatalogType.HIVE_METASTORE; import static io.trino.plugin.iceberg.IcebergFileFormat.ORC; public class IcebergConfig @@ -32,7 +32,7 @@ public class IcebergConfig private boolean useFileSizeFromMetadata = true; private int maxPartitionsPerWriter = 100; private boolean uniqueTableLocation; - private CatalogType catalogType = HIVE; + private CatalogType catalogType = HIVE_METASTORE; public CatalogType getCatalogType() { diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetastoreModule.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetastoreModule.java index 8638698c0978..443080ce1ca4 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetastoreModule.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetastoreModule.java @@ -17,7 +17,6 @@ import com.google.inject.Module; import io.airlift.configuration.AbstractConfigurationAwareModule; import io.trino.plugin.hive.metastore.HiveMetastore; -import io.trino.plugin.hive.metastore.MetastoreTypeConfig; import io.trino.plugin.hive.metastore.cache.CachingHiveMetastore; import io.trino.plugin.hive.metastore.cache.CachingHiveMetastoreModule; import io.trino.plugin.hive.metastore.cache.ForCachingHiveMetastore; @@ -29,6 +28,8 @@ import java.util.Optional; import static io.airlift.configuration.ConditionalModule.conditionalModule; +import static io.trino.plugin.iceberg.CatalogType.HIVE_METASTORE; +import static io.trino.plugin.iceberg.CatalogType.TESTING_FILE_METASTORE; import static java.util.Objects.requireNonNull; public class IcebergMetastoreModule @@ -49,8 +50,8 @@ protected void setup(Binder binder) install(new CachingHiveMetastoreModule()); } else { - bindMetastoreModule("thrift", new ThriftMetastoreModule()); - bindMetastoreModule("file", new FileMetastoreModule()); + bindMetastoreModule(HIVE_METASTORE, new ThriftMetastoreModule()); + bindMetastoreModule(TESTING_FILE_METASTORE, new FileMetastoreModule()); // TODO add support for Glue metastore } @@ -68,11 +69,11 @@ public MetastoreValidator(HiveMetastore metastore) } } - private void bindMetastoreModule(String name, Module module) + private void bindMetastoreModule(CatalogType catalogType, Module module) { install(conditionalModule( - MetastoreTypeConfig.class, - metastore -> name.equalsIgnoreCase(metastore.getMetastoreType()), + IcebergConfig.class, + config -> config.getCatalogType() == catalogType, module)); } } diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergModule.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergModule.java index b220e9f697e7..7ba247f1e197 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergModule.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergModule.java @@ -74,6 +74,7 @@ public void configure(Binder binder) binder.bind(FileFormatDataSourceStats.class).in(Scopes.SINGLETON); newExporter(binder).export(FileFormatDataSourceStats.class).withGeneratedName(); + // TODO inject table operations based on IcebergConfig.getCatalogType binder.bind(HiveTableOperationsProvider.class).in(Scopes.SINGLETON); binder.bind(IcebergFileWriterFactory.class).in(Scopes.SINGLETON); diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/TrinoCatalogFactory.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/TrinoCatalogFactory.java index ac66122c7a62..ca5b39413938 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/TrinoCatalogFactory.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/TrinoCatalogFactory.java @@ -75,9 +75,11 @@ public TrinoCatalogFactory( public TrinoCatalog create() { switch (catalogType) { - case HIVE: + case TESTING_FILE_METASTORE: + case HIVE_METASTORE: return new TrinoHiveCatalog(catalogName, memoizeMetastore(metastore, 1000), hdfsEnvironment, typeManager, tableOperationsProvider, trinoVersion, isUniqueTableLocation); - case UNKNOWN: + case GLUE: + // TODO not supported yet throw new TrinoException(NOT_SUPPORTED, "Unknown Trino Iceberg catalog type"); } throw new TrinoException(NOT_SUPPORTED, "Unsupported Trino Iceberg catalog type " + catalogType); diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/IcebergQueryRunner.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/IcebergQueryRunner.java index e871e915488d..88eb0b5e6b06 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/IcebergQueryRunner.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/IcebergQueryRunner.java @@ -85,7 +85,7 @@ public static DistributedQueryRunner createIcebergQueryRunner( queryRunner.installPlugin(new IcebergPlugin()); connectorProperties = new HashMap<>(ImmutableMap.copyOf(connectorProperties)); - connectorProperties.putIfAbsent("hive.metastore", "file"); + connectorProperties.putIfAbsent("iceberg.catalog.type", "TESTING_FILE_METASTORE"); connectorProperties.putIfAbsent("hive.metastore.catalog.dir", dataDir.toString()); queryRunner.createCatalog(ICEBERG_CATALOG, "iceberg", connectorProperties); diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConfig.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConfig.java index b496364d9df3..292347ba5bc6 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConfig.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergConfig.java @@ -23,8 +23,8 @@ import static io.airlift.configuration.testing.ConfigAssertions.assertRecordedDefaults; import static io.airlift.configuration.testing.ConfigAssertions.recordDefaults; import static io.trino.plugin.hive.HiveCompressionCodec.GZIP; -import static io.trino.plugin.iceberg.CatalogType.HIVE; -import static io.trino.plugin.iceberg.CatalogType.UNKNOWN; +import static io.trino.plugin.iceberg.CatalogType.GLUE; +import static io.trino.plugin.iceberg.CatalogType.HIVE_METASTORE; import static io.trino.plugin.iceberg.IcebergFileFormat.ORC; import static io.trino.plugin.iceberg.IcebergFileFormat.PARQUET; @@ -39,7 +39,7 @@ public void testDefaults() .setUseFileSizeFromMetadata(true) .setMaxPartitionsPerWriter(100) .setUniqueTableLocation(false) - .setCatalogType(HIVE)); + .setCatalogType(HIVE_METASTORE)); } @Test @@ -51,7 +51,7 @@ public void testExplicitPropertyMappings() .put("iceberg.use-file-size-from-metadata", "false") .put("iceberg.max-partitions-per-writer", "222") .put("iceberg.unique-table-location", "true") - .put("iceberg.catalog.type", "UNKNOWN") + .put("iceberg.catalog.type", "GLUE") .build(); IcebergConfig expected = new IcebergConfig() @@ -60,7 +60,7 @@ public void testExplicitPropertyMappings() .setUseFileSizeFromMetadata(false) .setMaxPartitionsPerWriter(222) .setUniqueTableLocation(true) - .setCatalogType(UNKNOWN); + .setCatalogType(GLUE); assertFullMapping(properties, expected); } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergPlugin.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergPlugin.java index 56fa8abecd45..6237de8b4ba0 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergPlugin.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergPlugin.java @@ -40,12 +40,26 @@ public void testThriftMetastore() factory.create( "test", Map.of( - "hive.metastore", "thrift", + "iceberg.catalog.type", "HIVE_METASTORE", "hive.metastore.uri", "thrift://foo:1234"), new TestingConnectorContext()) .shutdown(); } + @Test + public void testHiveMetastoreRejected() + { + ConnectorFactory factory = getConnectorFactory(); + + assertThatThrownBy(() -> factory.create( + "test", + Map.of( + "hive.metastore", "thrift", + "hive.metastore.uri", "thrift://foo:1234"), + new TestingConnectorContext())) + .hasMessageContaining("Error: Configuration property 'hive.metastore' was not used"); + } + @Test public void testGlueMetastore() { @@ -53,14 +67,14 @@ public void testGlueMetastore() assertThatThrownBy(() -> factory.create( "test", - Map.of("hive.metastore", "glue"), + Map.of("iceberg.catalog.type", "glue"), new TestingConnectorContext())) .hasMessageContaining("Explicit bindings are required and HiveMetastore is not explicitly bound"); assertThatThrownBy(() -> factory.create( "test", Map.of( - "hive.metastore", "glue", + "iceberg.catalog.type", "glue", "hive.metastore.uri", "thrift://foo:1234"), new TestingConnectorContext())) .hasMessageContaining("Error: Configuration property 'hive.metastore.uri' was not used"); @@ -75,7 +89,7 @@ public void testRecordingMetastore() factory.create( "test", Map.of( - "hive.metastore", "thrift", + "iceberg.catalog.type", "HIVE_METASTORE", "hive.metastore.uri", "thrift://foo:1234", "hive.metastore-recording-path", "/tmp"), new TestingConnectorContext()) @@ -85,7 +99,7 @@ public void testRecordingMetastore() assertThatThrownBy(() -> factory.create( "test", Map.of( - "hive.metastore", "glue", + "iceberg.catalog.type", "glue", "hive.metastore.glue.region", "us-east-2", "hive.metastore-recording-path", "/tmp"), new TestingConnectorContext())) From 0da3e53c911f53ff8df665cc0108f686781d869c Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Mon, 11 Oct 2021 15:24:01 +0200 Subject: [PATCH 2/2] empty