AWS: support force register table in GlueCatalog #6742

theoxu31 · 2023-02-04T00:35:45Z

Add support for registerTable in GlueCatalog.
Customizations:

allowing GlueDataCatalog registerTable API for exiting Table
remove the commit part in registerTable API to avoid creating new metadata file
copy the iceberg parameters at table level to glue storage descriptor level as well for consistency with other glue catalog tables.
allow s3 file io cross region call for glue catalog register table when using assumeRole

Reference: #4099

jackye1995 · 2023-02-04T01:07:37Z

Thanks for picking this up from me, ping a few people for review @amogh-jahagirdar @singhpk234 @rajarshisarkar @aajisaka @JonasJ-ap

jackye1995 · 2023-02-04T01:10:53Z

aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java

  }

+  @Override
+  public org.apache.iceberg.Table registerTable(TableIdentifier identifier, String metadataFileLocation) {


I think we might want to have a feature flag like glue.register-table.create-new-metadata (for the lack of a better name) in AwsProperties to distinguish 2 behaviors, to either create a new metadata file or not. If the flag is true (by default), it can call the base class method directly.

jackye1995 · 2023-02-04T01:12:20Z

aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java

+          .tableInput(tableInput)
+          .build());
+    } catch (software.amazon.awssdk.services.glue.model.AlreadyExistsException e) {
+      glue.updateTable(UpdateTableRequest.builder()


Similarly, I think we should at least make this a feature flag like glue.register-table.replace-if-exists.

We could further argue if we should make this an API feature or not, but that is subject to debate. Any thoughts?

jackye1995 · 2023-02-04T01:14:24Z

aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java

+    TableInput tableInput = TableInput.builder()
+        .name(IcebergToGlueConverter.getTableName(identifier, awsProperties.glueCatalogSkipNameValidation()))
+        .tableType(GlueTableOperations.GLUE_EXTERNAL_TABLE_TYPE)
+        .parameters(tableParameters)


I think here we need to still get the metadata and use the IcebergToGlueConverter to get the merged schema for display.

ajantha-bhat · 2023-02-04T06:43:22Z

remove the commit part in registerTable API to avoid creating new metadata file

#6591 already fixed this part for Glue too right?

jackye1995 · 2023-02-04T08:06:10Z

#6591 already fixed this part for Glue too right?

great, did not notice that one!

In that case I think the only missing feature is just replace

ajantha-bhat · 2023-02-04T10:12:43Z

allowing GlueDataCatalog registerTable API for exiting Table

There is also related on going work
#5327

jackye1995 · 2023-02-04T17:55:09Z

Nice, I anticipated similar concerns as in that thread, that's why I'd like to just put it up and see how the community reacts to this.

I think the conversation there was around the fact that registerTable is used for a recovery use case, so there is no requirement for atomic operation and the original metadata location might be broken.

What Theo is trying to achieve (based on my understanding) is a use case of continuous registration, where a user sends notification of the latest metadata location, and then the metadata location is updated for an existing table in Glue. This is developed for migration use cases of, for example, HiveCatalog or HadoopCatalog to GlueCatalog.

In this use case, atmoic switch of metadata location is a requirement, compared to a drop + register case. And it can clearly be achieved through calling glue.updateTable with all the right information. I think we would like to know if that is something worth adding to the Iceberg OSS, or it needs to just remain as a custom logic.

I think it has benefit in OSS, as people naturally think of using registerTable when talking about this use case, and as we have so many catalog offerings, it's worth supporting cross-catalog migration explicitly.

Any thoughts? @RussellSpitzer @yabola @flyrain @szehon-ho @rdblue

yabola · 2023-02-05T02:19:16Z

@jackye1995 Thanks for pinging me. I agree with you : there is no requirement in a recovery use case, but this can be a requirement in automatic switch of metadata location. But I am not sure if it is a custom logic. Looking forward to other people's perspective . If it makes sense, I can continue to complete my PR.

ajantha-bhat · 2023-02-05T17:21:27Z

I think it has benefit in OSS, as people naturally think of using registerTable when talking about this use case, and as we have so many catalog offerings, it's worth supporting cross-catalog migration explicitly.

@jackye1995: Totally agree that we need to support cross catalog migration.
So, long back I raised #5492 and got a comment that it is better to handle this in a separate project (was also discussed this in the previous Iceberg sync). So, As a side project I am working on it (https://github.com/ajantha-bhat/iceberg-catalog-migrator/blob/first/README.md). In a week or two I can publish it as an open source project under projectNessie repo.

jackye1995 · 2023-02-05T18:24:00Z

I briefly read the project you linked, that's very cool CLI! But we are not really trying to build a new migration project out of it, the ask is much simpler.

What I want to get out of the discussion is really which way we go out of the following:

atomic replace can be a catalog-specific feature for registerTable, such that a glue.force-register-table flag could be added to support it
an API change in Iceberg catalog can be performed to make this a generic feature, such as adding a force boolean flag to registerTable
it should not be a part of Iceberg core catalog API at all

jackye1995 · 2023-02-06T17:50:33Z

aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java

+          .databaseName(IcebergToGlueConverter.getDatabaseName(identifier,
+              awsProperties.glueCatalogSkipNameValidation()))
+          .tableInput(tableInput)
+          .build());


this should first get the last table version to avoid commit conflict

I think glue catalog already handle this internally?

If we call glue.getTable to get the table version first then call glue.updateTable with nextVersionId it will cause concurrentModificationException.

Yes this is what I mean. It needs to explicitly pass in the version number of the current version to ensure atomic update.

Ack, will update

aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java

…c transaction

jackye1995 · 2023-02-07T21:55:02Z

aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java

+      glue.createTable(
+          CreateTableRequest.builder().databaseName(databaseName).tableInput(tableInput).build());
+    } catch (software.amazon.awssdk.services.glue.model.AlreadyExistsException e) {
+      if (awsProperties.glueCatalogForceRegisterTable()) {


I think we can do something better to keep the original behavior when the flag is off:

if (!awsProperties.glueCatalogForceRegisterTable()) { return super.registerTable(.....); } // the replace-based logic below ...

theoxu31 · 2023-02-16T01:28:38Z

Resolved comments, pinging people for review @jackye1995 @yabola @amogh-jahagirdar @singhpk234 @rajarshisarkar @aajisaka @JonasJ-ap

amogh-jahagirdar

Overall looking good, I think there's some fixes we need to do in exception handling and the tests we are adding.

amogh-jahagirdar · 2023-02-16T04:39:36Z

aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java

-    AssertHelpers.assertThrows(
-        "should fail to rename",
-        ValidationException.class,
-        "Input Glue table is not an iceberg table",
-        () ->
-            glueCatalog.renameTable(
-                TableIdentifier.of(namespace, tableName),
-                TableIdentifier.of(namespace, newTableName)));


Why is this assertion removed? It looks like it's for rename

This assertion was removed due to the logic change in renameTable that allows rename table to use the previous table's Iceberg Properties (metadata location) the related integration test would always fail at this assert.

amogh-jahagirdar · 2023-02-16T04:43:36Z

aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java

-    Table table = glueCatalog.loadTable(identifier);
+    Table table = glueCatalogWithForceRegisterTable.loadTable(identifier);
    String metadataLocation = ((BaseTable) table).operations().current().metadataFileLocation();
-    Assertions.assertThatThrownBy(() -> glueCatalog.registerTable(identifier, metadataLocation))
-        .isInstanceOf(AlreadyExistsException.class);
-    Assertions.assertThat(glueCatalog.dropTable(identifier, true)).isTrue();
-    Assertions.assertThat(glueCatalog.dropNamespace(Namespace.of(namespace))).isTrue();
+    Assertions.assertThat(glueCatalogWithForceRegisterTable.dropTable(identifier, false)).isTrue();
+    Table registeredTable =
+        glueCatalogWithForceRegisterTable.registerTable(identifier, metadataLocation);
+    Assertions.assertThat(registeredTable).isNotNull();
+
+    GetTableResponse response =
+        glue.getTable(GetTableRequest.builder().databaseName(namespace).name(tableName).build());
+    Assert.assertEquals(
+        "external table type is set after register",
+        "EXTERNAL_TABLE",
+        response.table().tableType());
+    String actualMetadataLocation =
+        response.table().parameters().get(BaseMetastoreTableOperations.METADATA_LOCATION_PROP);
+    Assert.assertEquals(
+        "metadata location should be updated with registerTable call",
+        metadataLocation,
+        actualMetadataLocation);
+
+    // commit new transaction, should create a new metadata file
+    DataFile dataFile =
+        DataFiles.builder(partitionSpec)
+            .withPath("/path/to/data-a.parquet")
+            .withFileSizeInBytes(10)
+            .withRecordCount(1)
+            .build();
+    table.newAppend().appendFile(dataFile).commit();
+
+    metadataLocation = ((BaseTable) table).operations().current().metadataFileLocation();
+    // update metadata location
+    glueCatalogWithForceRegisterTable.registerTable(identifier, metadataLocation);
+    response =
+        glue.getTable(GetTableRequest.builder().databaseName(namespace).name(tableName).build());
+    String updatedMetadataLocation =
+        response.table().parameters().get(BaseMetastoreTableOperations.METADATA_LOCATION_PROP);
+    Assert.assertEquals(
+        "metadata location should be updated with registerTable call",
+        metadataLocation,
+        updatedMetadataLocation);
+    Assert.assertEquals("Table Version should be updated", "2", response.table().versionId());


I think we'll want separate tests for testRegisterWhenTableExists, one is when force registration is enabled and one when not. we still want to validate the case when force is false works as expected but this change seems to be removing the validating the previous case. Let me know if I missed something when reading the code!

Yes that's a good call out, I have separated into two tests in the new revision commits, thanks!

amogh-jahagirdar · 2023-02-16T04:46:01Z

aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java

+      throw new NoSuchNamespaceException(
+          e, "Namespace %s is not found in Glue", identifier.namespace());


The exception handling seems off, can't EntityNotFoundException also be thrown when the table is not found?

This exception handling is meant for catching exceptions for glue.createTable calls only, for any exception during the getTable & updateTable calls we should have it throw exceptions as expected, thoughts?
Also I think the EntityNotFoundException won't be thrown for table not found in the case of Table AlreadyExistsException.

ChristinaTech · 2023-05-07T02:48:46Z

aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java

-    this.catalogProperties = ImmutableMap.copyOf(properties);
+    this.catalogProperties = new HashMap<>();
+    catalogProperties.putAll(properties);


Making CatalogProperties non-immutable in order to effectively side-load information into the AWS client factory post-initialization is a dangerous precedent to set. In addition, it doesn't seem to actually accomplish anything besides allowing the GlueCatalog to suddenly switch regions post-initialization, which is likely to introduce some dangerous side effects.

ChristinaTech · 2023-05-07T02:50:21Z

aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java

+
+    String factoryImpl =
+        PropertyUtil.propertyAsString(catalogProperties, AwsProperties.CLIENT_FACTORY, null);
+    if (factoryImpl != null && factoryImpl.equals(AssumeRoleAwsClientFactory.class.getName())) {


It hurts extensibility to make logic specific to a particular implementation class. If, for example, a customer needs to extend AssumeRoleAwsClientFactory to add some functionality for their own use, this logic will break as the class name will no longer match.

ChristinaTech · 2023-05-07T02:57:04Z

aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java

+      String catalogFileIORegion = awsProperties.getGlueCatalogFileIORegion();
+      if (catalogFileIORegion != null) {
+        catalogProperties.put(AwsProperties.CLIENT_ASSUME_ROLE_REGION, catalogFileIORegion);
+      }


Can't this logic and the associated parameter be removed and replaced with just setting AwsProperties.CLIENT_ASSUME_ROLE_REGION directly? This change is not in any way scoped to this call or AWS service, it effectively creates a case where what region the GlueCatalog is calling can suddenly change post-initialization after the first time registerTable is called.

ChristinaTech · 2023-05-07T03:03:31Z

aws/src/main/java/org/apache/iceberg/aws/AwsProperties.java

+  public static final boolean GLUE_CATALOG_FORCE_REGISTER_TABLE_DEFAULT = false;
+
+  /** Configure the Glue Catalog S3 FileIO Region to allow cross region s3 access */
+  public static final String GLUE_CATALOG_FILE_IO_REGION = "glue.catalog-file-io-region";


So we already have client.region which sets the region for all services. I am going to be introducing a change in the near future that will allow setting per-region for the default AWS Client Factory (and we can extend it to the Assume Role client factory as well), so we probably want a more general per-service naming scheme. Based on how existing parameters are formatted where glue. and s3. are already established prefixes, most likely something like: glue.region, s3.region, kms.region, etc...

ChristinaTech · 2023-05-07T03:26:18Z

aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java

+    TableOperations ops = newTableOps(identifier);
+    InputFile metadataFile = ops.io().newInputFile(metadataFileLocation);
+    TableMetadata metadata = TableMetadataParser.read(ops.io(), metadataFile);
+
+    Map<String, String> tableParameters =
+        ImmutableMap.of(
+            BaseMetastoreTableOperations.TABLE_TYPE_PROP,
+            BaseMetastoreTableOperations.ICEBERG_TABLE_TYPE_VALUE.toLowerCase(Locale.ENGLISH),
+            BaseMetastoreTableOperations.METADATA_LOCATION_PROP,
+            metadataFileLocation);
+
+    String databaseName =
+        IcebergToGlueConverter.getDatabaseName(
+            identifier, awsProperties.glueCatalogSkipNameValidation());
+    String tableName =
+        IcebergToGlueConverter.getTableName(
+            identifier, awsProperties.glueCatalogSkipNameValidation());
+
+    TableInput tableInput =
+        TableInput.builder()
+            .applyMutation(
+                builder ->
+                    IcebergToGlueConverter.setTableInputInformation(
+                        builder, metadata, tableParameters))
+            .name(tableName)
+            .tableType(GlueTableOperations.GLUE_EXTERNAL_TABLE_TYPE)
+            .parameters(tableParameters)
+            .build();
+
+    try {
+      glue.createTable(
+          CreateTableRequest.builder().databaseName(databaseName).tableInput(tableInput).build());
+    } catch (software.amazon.awssdk.services.glue.model.AlreadyExistsException e) {
+      GetTableResponse response =
+          glue.getTable(
+              GetTableRequest.builder().databaseName(databaseName).name(tableName).build());
+      String versionId = response.table().versionId();
+      glue.updateTable(
+          UpdateTableRequest.builder()
+              .databaseName(databaseName)
+              .tableInput(tableInput)
+              .versionId(versionId)
+              .build());
+    } catch (EntityNotFoundException e) {


So this logic is a combination of a fork of the logic in BaseMetastoreCatalog.registerTable and GlueTableOperations.persistGlueTable. As GlueTableOperations also has access to AwsProperties, I would recommend refactoring the logic in GlueTableOperations so that persistGlueTable can conditionally fall back to its update mode as that improves code reuse.

If the concern is the creation of an extra metadata file, it looks like GlueTableOperations already checks whether it needs to during the writeNewMetadataIfRequired function and considering tableMetadata is always set for registerTable, it will never choose to write a new metadata file anyways.

github-actions · 2024-08-25T00:14:59Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

github-actions · 2024-09-12T00:14:51Z

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

support registerTable in GlueCatalog

73ecc5d

github-actions bot added the AWS label Feb 4, 2023

remove unnessary lines

4ac803c

jackye1995 reviewed Feb 4, 2023

View reviewed changes

jackye1995 reviewed Feb 6, 2023

View reviewed changes

resolve comments in pr

3509f3a

jackye1995 reviewed Feb 7, 2023

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java Show resolved Hide resolved

jackye1995 changed the title ~~support registerTable in GlueCatalog~~ AWS: support force register table in GlueCatalog Feb 7, 2023

add feature flag to registerTable & specify versionId to ensure atomi…

3c25e64

…c transaction

jackye1995 reviewed Feb 7, 2023

View reviewed changes

resolve comments to optimize the flag behavior in registerTable

8e5349b

amogh-jahagirdar reviewed Feb 16, 2023

View reviewed changes

theoxu31 and others added 4 commits April 7, 2023 01:18

support glue endpoint for assumeRoleAwsClientFactory

043b8a6

support pass in table paramters to storagedescriptor

4bdb165

allow s3 file io cross region call for glue catalog register table

b38a928

Merge branch 'master' into glue-register-table

dd9fdc4

ChristinaTech suggested changes May 7, 2023

View reviewed changes

krvikash mentioned this pull request Jun 27, 2023

Core: Support registerTable with REST session catalog #6512

Merged

github-actions bot added the stale label Aug 25, 2024

github-actions bot closed this Sep 12, 2024

		throw new NoSuchNamespaceException(
		e, "Namespace %s is not found in Glue", identifier.namespace());

AWS: support force register table in GlueCatalog #6742

AWS: support force register table in GlueCatalog #6742

Uh oh!

Conversation

theoxu31 commented Feb 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jackye1995 commented Feb 4, 2023

Uh oh!

jackye1995 Feb 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajantha-bhat commented Feb 4, 2023

Uh oh!

jackye1995 commented Feb 4, 2023

Uh oh!

ajantha-bhat commented Feb 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jackye1995 commented Feb 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yabola commented Feb 5, 2023

Uh oh!

ajantha-bhat commented Feb 5, 2023

Uh oh!

jackye1995 commented Feb 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theoxu31 commented Feb 16, 2023

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 25, 2024

Uh oh!

github-actions bot commented Sep 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

theoxu31 commented Feb 4, 2023 •

edited

Loading

jackye1995 Feb 4, 2023 •

edited

Loading

ajantha-bhat commented Feb 4, 2023 •

edited

Loading

jackye1995 commented Feb 4, 2023 •

edited

Loading