Support hive.metastore.glue.skip-archive in v1 Glue

trinodb · Oct 31, 2024 · 00ccd3c · 00ccd3c
1 parent 51915cb
commit 00ccd3c
Show file tree

Hide file tree

Showing 9 changed files with 37 additions and 43 deletions.
diff --git a/docs/src/main/sphinx/object-storage/metastores.md b/docs/src/main/sphinx/object-storage/metastores.md
@@ -436,16 +436,11 @@ described with the following additional property:
 * - Property name
   - Description
   - Default
-* - `iceberg.glue.skip-archive`
-  - Skip archiving an old table version when creating a new version in a commit.
-    See [AWS Glue Skip
-    Archive](https://iceberg.apache.org/docs/latest/aws/#skip-archive).
-  - `true`
 * - `iceberg.glue.cache-table-metadata`
   - While updating the table in AWS Glue, store the table metadata with the
     purpose of accelerating `information_schema.columns` and
     `system.metadata.table_comments` queries.
-  - `true` 
+  - `true`
   :::
 
 ## Iceberg-specific metastores

diff --git a/...no-hive/src/main/java/io/trino/plugin/hive/metastore/glue/v1/GlueHiveMetastoreConfig.java b/...no-hive/src/main/java/io/trino/plugin/hive/metastore/glue/v1/GlueHiveMetastoreConfig.java
@@ -17,6 +17,7 @@
 import io.airlift.configuration.ConfigDescription;
 import io.airlift.configuration.ConfigSecuritySensitive;
 import io.airlift.configuration.DefunctConfig;
+import io.airlift.configuration.LegacyConfig;
 import jakarta.annotation.PostConstruct;
 import jakarta.validation.constraints.Max;
 import jakarta.validation.constraints.Min;
@@ -48,6 +49,7 @@ public class GlueHiveMetastoreConfig
     private int readStatisticsThreads = 5;
     private int writeStatisticsThreads = 20;
     private boolean assumeCanonicalPartitionKeys;
+    private boolean skipArchive;
 
     public Optional<String> getGlueRegion()
     {
@@ -317,6 +319,20 @@ public GlueHiveMetastoreConfig setAssumeCanonicalPartitionKeys(boolean assumeCan
         return this;
     }
 
+    public boolean isSkipArchive()
+    {
+        return skipArchive;
+    }
+
+    @Config("hive.metastore.glue.skip-archive")
+    @LegacyConfig("iceberg.glue.skip-archive")
+    @ConfigDescription("Skip archiving an old table version when updating a table in the Glue metastore")
+    public GlueHiveMetastoreConfig setSkipArchive(boolean skipArchive)
+    {
+        this.skipArchive = skipArchive;
+        return this;
+    }
+
     @PostConstruct
     public void validate()
     {

diff --git a/.../trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/v1/GlueMetastoreModule.java b/.../trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/v1/GlueMetastoreModule.java
@@ -94,7 +94,18 @@ protected void setup(Binder binder)
     @ProvidesIntoSet
     @Singleton
     @ForGlueHiveMetastore
-    public RequestHandler2 createRequestHandler(OpenTelemetry openTelemetry)
+    public RequestHandler2 createSkipArchiveRequestHandler(GlueHiveMetastoreConfig config)
+    {
+        if (!config.isSkipArchive()) {
+            return new RequestHandler2() {};
+        }
+        return new SkipArchiveRequestHandler();
+    }
+
+    @ProvidesIntoSet
+    @Singleton
+    @ForGlueHiveMetastore
+    public RequestHandler2 createTelemetryRequestHandler(OpenTelemetry openTelemetry)
     {
         return AwsSdkTelemetry.builder(openTelemetry)
                 .setCaptureExperimentalSpanAttributes(true)

diff --git a/...talog/glue/SkipArchiveRequestHandler.java → ...re/glue/v1/SkipArchiveRequestHandler.java b/...talog/glue/SkipArchiveRequestHandler.java → ...re/glue/v1/SkipArchiveRequestHandler.java
@@ -11,7 +11,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package io.trino.plugin.iceberg.catalog.glue;
+package io.trino.plugin.hive.metastore.glue.v1;
 
 import com.amazonaws.AmazonWebServiceRequest;
 import com.amazonaws.handlers.RequestHandler2;

diff --git a/...ive/src/test/java/io/trino/plugin/hive/metastore/glue/v1/TestGlueHiveMetastoreConfig.java b/...ive/src/test/java/io/trino/plugin/hive/metastore/glue/v1/TestGlueHiveMetastoreConfig.java
@@ -46,6 +46,7 @@ public void testDefaults()
                 .setPartitionSegments(5)
                 .setGetPartitionThreads(20)
                 .setAssumeCanonicalPartitionKeys(false)
+                .setSkipArchive(false)
                 .setReadStatisticsThreads(5)
                 .setWriteStatisticsThreads(20));
     }
@@ -72,6 +73,7 @@ public void testExplicitPropertyMapping()
                 .put("hive.metastore.glue.partitions-segments", "10")
                 .put("hive.metastore.glue.get-partition-threads", "42")
                 .put("hive.metastore.glue.assume-canonical-partition-keys", "true")
+                .put("hive.metastore.glue.skip-archive", "true")
                 .put("hive.metastore.glue.read-statistics-threads", "42")
                 .put("hive.metastore.glue.write-statistics-threads", "43")
                 .buildOrThrow();
@@ -95,6 +97,7 @@ public void testExplicitPropertyMapping()
                 .setPartitionSegments(10)
                 .setGetPartitionThreads(42)
                 .setAssumeCanonicalPartitionKeys(true)
+                .setSkipArchive(true)
                 .setReadStatisticsThreads(42)
                 .setWriteStatisticsThreads(43);
 

diff --git a/...-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/IcebergGlueCatalogConfig.java b/...-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/IcebergGlueCatalogConfig.java
@@ -14,12 +14,10 @@
 package io.trino.plugin.iceberg.catalog.glue;
 
 import io.airlift.configuration.Config;
-import io.airlift.configuration.ConfigDescription;
 
 public class IcebergGlueCatalogConfig
 {
     private boolean cacheTableMetadata = true;
-    private boolean skipArchive = true;
 
     public boolean isCacheTableMetadata()
     {
@@ -32,17 +30,4 @@ public IcebergGlueCatalogConfig setCacheTableMetadata(boolean cacheTableMetadata
         this.cacheTableMetadata = cacheTableMetadata;
         return this;
     }
-
-    public boolean isSkipArchive()
-    {
-        return skipArchive;
-    }
-
-    @Config("iceberg.glue.skip-archive")
-    @ConfigDescription("Skip archiving an old table version when creating a new version in a commit")
-    public IcebergGlueCatalogConfig setSkipArchive(boolean skipArchive)
-    {
-        this.skipArchive = skipArchive;
-        return this;
-    }
 }
diff --git a/...-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/IcebergGlueCatalogModule.java b/...-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/IcebergGlueCatalogModule.java
@@ -14,7 +14,6 @@
 package io.trino.plugin.iceberg.catalog.glue;
 
 import com.amazonaws.auth.AWSCredentialsProvider;
-import com.amazonaws.handlers.RequestHandler2;
 import com.amazonaws.services.glue.model.Table;
 import com.google.inject.Binder;
 import com.google.inject.Key;
@@ -37,7 +36,6 @@
 
 import static com.google.inject.multibindings.Multibinder.newSetBinder;
 import static com.google.inject.multibindings.OptionalBinder.newOptionalBinder;
-import static io.airlift.configuration.ConditionalModule.conditionalModule;
 import static io.airlift.configuration.ConfigBinder.configBinder;
 import static org.weakref.jmx.guice.ExportBinder.newExporter;
 
@@ -48,6 +46,7 @@ public class IcebergGlueCatalogModule
     protected void setup(Binder binder)
     {
         configBinder(binder).bindConfig(GlueHiveMetastoreConfig.class);
+        configBinder(binder).bindConfigDefaults(GlueHiveMetastoreConfig.class, config -> config.setSkipArchive(true));
         configBinder(binder).bindConfig(IcebergGlueCatalogConfig.class);
         binder.bind(GlueMetastoreStats.class).in(Scopes.SINGLETON);
         newExporter(binder).export(GlueMetastoreStats.class).withGeneratedName();
@@ -56,11 +55,6 @@ protected void setup(Binder binder)
         binder.bind(TrinoCatalogFactory.class).to(TrinoGlueCatalogFactory.class).in(Scopes.SINGLETON);
         newExporter(binder).export(TrinoCatalogFactory.class).withGeneratedName();
 
-        install(conditionalModule(
-                IcebergGlueCatalogConfig.class,
-                IcebergGlueCatalogConfig::isSkipArchive,
-                internalBinder -> newSetBinder(internalBinder, RequestHandler2.class, ForGlueHiveMetastore.class).addBinding().toInstance(new SkipArchiveRequestHandler())));
-
         // Required to inject HiveMetastoreFactory for migrate procedure
         binder.bind(Key.get(boolean.class, HideDeltaLakeTables.class)).toInstance(false);
         newOptionalBinder(binder, Key.get(new TypeLiteral<Predicate<Table>>() {}, ForGlueHiveMetastore.class))

diff --git a/...berg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogConfig.java b/...berg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogConfig.java
@@ -28,21 +28,18 @@ public class TestIcebergGlueCatalogConfig
     public void testDefaults()
     {
         assertRecordedDefaults(recordDefaults(IcebergGlueCatalogConfig.class)
-                .setCacheTableMetadata(true)
-                .setSkipArchive(true));
+                .setCacheTableMetadata(true));
     }
 
     @Test
     public void testExplicitPropertyMapping()
     {
         Map<String, String> properties = ImmutableMap.<String, String>builder()
                 .put("iceberg.glue.cache-table-metadata", "false")
-                .put("iceberg.glue.skip-archive", "false")
                 .buildOrThrow();
 
         IcebergGlueCatalogConfig expected = new IcebergGlueCatalogConfig()
-                .setCacheTableMetadata(false)
-                .setSkipArchive(false);
+                .setCacheTableMetadata(false);
 
         assertFullMapping(properties, expected);
     }

diff --git a/...g/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestingIcebergGlueCatalogModule.java b/...g/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestingIcebergGlueCatalogModule.java
@@ -14,7 +14,6 @@
 package io.trino.plugin.iceberg.catalog.glue;
 
 import com.amazonaws.auth.AWSCredentialsProvider;
-import com.amazonaws.handlers.RequestHandler2;
 import com.amazonaws.services.glue.model.Table;
 import com.google.inject.Binder;
 import com.google.inject.Key;
@@ -32,9 +31,7 @@
 
 import java.util.function.Predicate;
 
-import static com.google.inject.multibindings.Multibinder.newSetBinder;
 import static com.google.inject.multibindings.OptionalBinder.newOptionalBinder;
-import static io.airlift.configuration.ConditionalModule.conditionalModule;
 import static io.airlift.configuration.ConfigBinder.configBinder;
 import static java.util.Objects.requireNonNull;
 import static org.weakref.jmx.guice.ExportBinder.newExporter;
@@ -53,6 +50,7 @@ public TestingIcebergGlueCatalogModule(AWSGlueAsyncAdapterProvider awsGlueAsyncA
     protected void setup(Binder binder)
     {
         configBinder(binder).bindConfig(GlueHiveMetastoreConfig.class);
+        configBinder(binder).bindConfigDefaults(GlueHiveMetastoreConfig.class, config -> config.setSkipArchive(true));
         configBinder(binder).bindConfig(IcebergGlueCatalogConfig.class);
         binder.bind(GlueMetastoreStats.class).in(Scopes.SINGLETON);
         newExporter(binder).export(GlueMetastoreStats.class).withGeneratedName();
@@ -62,11 +60,6 @@ protected void setup(Binder binder)
         newExporter(binder).export(TrinoCatalogFactory.class).withGeneratedName();
         binder.bind(AWSGlueAsyncAdapterProvider.class).toInstance(awsGlueAsyncAdapterProvider);
 
-        install(conditionalModule(
-                IcebergGlueCatalogConfig.class,
-                IcebergGlueCatalogConfig::isSkipArchive,
-                internalBinder -> newSetBinder(internalBinder, RequestHandler2.class, ForGlueHiveMetastore.class).addBinding().toInstance(new SkipArchiveRequestHandler())));
-
         // Required to inject HiveMetastoreFactory for migrate procedure
         binder.bind(Key.get(boolean.class, HideDeltaLakeTables.class)).toInstance(false);
         newOptionalBinder(binder, Key.get(new TypeLiteral<Predicate<Table>>() {}, ForGlueHiveMetastore.class))