Fix iceberg table creation using glue metastore #20699

pratyakshsharma · 2023-08-29T09:42:12Z

Description

Fix iceberg table creation using glue metastore.

Motivation and Context

#18741

Impact

No impact.

Test Plan

Manual testing using self AWS account.

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Iceberg Changes
* Fix iceberg table creation using glue metastore

github-actions · 2023-08-29T10:06:26Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff e4d8fc1...2c67fe8.

No notifications.

steveburnett

LGTM! (docs) One small suggestion.

steveburnett · 2023-08-29T19:06:37Z

presto-docs/src/main/sphinx/connector/iceberg.rst

@@ -355,6 +355,12 @@ Create an Iceberg table with Iceberg format version 2::
      format_version = '2'
    )

+.. note::
+
+    `GlueHiveMetastore` makes use of DynamoDb based locking mechanism. Please assign a policy similar to the below to the IAM user being used for creating table using glue catalog::


Suggested change

`GlueHiveMetastore` makes use of DynamoDb based locking mechanism. Please assign a policy similar to the below to the IAM user being used for creating table using glue catalog::

`GlueHiveMetastore` makes use of DynamoDb based locking mechanism. Please assign a policy similar to the one shown below to the IAM user being used for creating tables using a Glue catalog::

pratyakshsharma · 2023-09-05T12:51:44Z

@yingsu00 This is good to review, only need to squash the commits. I will do that once I receive review comments.

imjalpreet

@pratyakshsharma Thank you for the contribution, I have commented my views on the changes, let me know your thoughts.

imjalpreet · 2023-09-07T06:47:46Z

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/glue/GlueHiveMetastore.java

+//        properties.put("lock-impl", "org.apache.iceberg.aws.glue.DynamoLockManager");
+        properties.put("lock.table", tableName);
+        properties.put("lock.acquire-timeout-ms", "300000");
+        LockManager lockManager = metastoreContext.getLockManager();


I have some concerns with the current implementation, this lock method is a common implementation for any table format, and introducing an iceberg dependency doesn't look like an ideal solution. If we look at the implementation for Hive Metastore, it is generic. Can we brainstorm if it would be possible to do something similar in this case as well?

In addition to that, this also introduces iceberg dependency in the hive connector.

Need to see if a similar LockManager class is available in aws-sdk for java.

imjalpreet · 2023-09-07T06:51:11Z

presto-hive-metastore/pom.xml

+        <dependency>
+            <groupId>software.amazon.awssdk</groupId>
+            <artifactId>dynamodb</artifactId>
+            <version>2.17.131</version>


Presto currently makes use of AWS SDK 1.x. I think we should use the same version here as well otherwise it could result in conflicts and difficulty to maintain. We can migrate to AWS SDK 2.x collectively as a project once it is prioritized.

So the idea behind introducing 2.x dependencies was latest iceberg code [1.3.1] is making use of 2.x sdk. So adding 1.x version for this dependency might have resulted in issues. Moreover, Trino also currently has both the dependencies, they are adding the 2.x dependencies as bom. So I don't think it should create a major issue. What do you think?
If you still strongly feel we should proceed with 1.x only for now, I can rework these imports. Let me know either ways.

imjalpreet · 2023-09-07T06:52:52Z

presto-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/MetastoreContext.java

@@ -38,6 +40,9 @@ public class MetastoreContext
    // a new MetastoreContext from either an existing MetastoreContext or callers
    // that only have a handle to the provider (e.g. SemiTransactionalHiveMetastore)
    private final ColumnConverterProvider columnConverterProvider;
+    private String metadataLocation;
+    private String commitLockEntryId;
+    private LockManager lockManager;


Similar concern, we are introducing an iceberg dependency in a shared class.

pratyakshsharma · 2023-09-20T20:47:25Z

Trino is currently using glue client from 1.x AWS SDK for java for iceberg connector. They are not implementing any locks for concurrent modifications in the case of glue metastore. Iceberg makes use of optimistic concurrency control for multiple writers, and currently trino only takes lock on thrift hive metastore to take care of such scenarios.
Read more about it here - https://www.starburst.io/blog/trino-on-ice-iii-iceberg-concurrency-model-snapshots-and-the-iceberg-spec/

References -
trinodb/trino#16762
trinodb/trino#13199

yingsu00

@pratyakshsharma I don't think this is the correct fix. The failure was because the "transaction" object created in IcebergXXXMetadata was wrong. It should be created as
transaction = createTableTransaction(tableName, operations, metadata); but instead, it was not. Therefore the the TransactionType was not CREATE_TABLE, but just SIMPLE. Can you please verify how the transaction was created? Was it using IcebergNativeMetadata or IcebergHiveMetadata? Was beginCreateTable() executed? Was the transaction created via newCreateTableTransaction() or just icebergTable.newTransaction()?

pratyakshsharma · 2023-10-11T15:06:31Z

@yingsu00 I checked and the transaction is of type CREATE_TABLE only and the flow executes via IcebergHiveMetadata. Also I did not understand the statement - "The failure was because the "transaction" object created in IcebergXXXMetadata was wrong." How is transaction object the cause of the failure? The failure is because of lock method not implemented in GlueHiveMetastore originally.

agrawalreetika · 2023-10-30T07:18:16Z

...o-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/ExtendedHiveMetastore.java

@@ -133,12 +132,12 @@ List<PartitionNameWithVersion> getPartitionNamesWithVersionByFilter(

    default long lock(MetastoreContext metastoreContext, String databaseName, String tableName)
    {
-        throw new NotSupportedException("Lock is not supported by default");
+        return 1L;


I think we can keep the default as it is in this interface and then handle lock & unlock in Glue implementation respectively instead.

And for https://github.com/prestodb/presto/blob/master/presto-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/ExtendedHiveMetastore.java#L134
We can make this Optional and return empty for Glue.
Let me know what do you think?

Yes, I agree. Let's keep the interface as-is since we don't want to allow pass-through by default in case of other metastores. We can override this in Glue Hive Metastore and allow pass-through.

Also, let's add some details about the verifications that you did to showcase we can pass through in the case of Glue Metastore

@imjalpreet No details are needed as such I guess. The tests were all performed for concurrency issues. Iceberg table creation simply works when we bypass the check for glue case. Let me know if you think otherwise.

yingsu00

@pratyakshsharma There are test failures and I think it's because of the test issue. Created #21346. Will you be able to fix it?

yingsu00

@pratyakshsharma Will you be able to add a test?

need a test

yingsu00 · 2023-11-09T07:39:25Z

@pratyakshsharma Will you be able to add a test?

As discussed offline, @pratyakshsharma will add the test in our internal repo since it requires Glue account.

imjalpreet

LGTM 👍🏼

@pratyakshsharma can you please add some screenshots if possible of your tests with Glue after this change for future reference?

pratyakshsharma requested review from shrinidhijoshi, a team, vinothchandar and 7c00 as code owners August 29, 2023 09:42

pratyakshsharma requested a review from presto-oss August 29, 2023 09:42

pratyakshsharma force-pushed the iceberg-glue-lock-issue branch from 3d2a0d5 to 666c739 Compare August 29, 2023 09:48

steveburnett reviewed Aug 29, 2023

View reviewed changes

pratyakshsharma force-pushed the iceberg-glue-lock-issue branch 2 times, most recently from c2b839d to e5bc97d Compare September 5, 2023 11:39

pratyakshsharma force-pushed the iceberg-glue-lock-issue branch from e5bc97d to 92deb73 Compare September 6, 2023 20:11

yingsu00 requested review from imjalpreet and agrawalreetika September 7, 2023 06:39

yingsu00 added the iceberg Apache Iceberg related label Sep 7, 2023

imjalpreet requested changes Sep 7, 2023

View reviewed changes

yingsu00 requested changes Sep 21, 2023

View reviewed changes

tdcmeehan assigned pratyakshsharma Sep 25, 2023

pratyakshsharma force-pushed the iceberg-glue-lock-issue branch from 92deb73 to 6b8429a Compare October 26, 2023 15:03

agrawalreetika requested changes Oct 30, 2023

View reviewed changes

Bypass metastore locking in Glue

2c67fe8

pratyakshsharma force-pushed the iceberg-glue-lock-issue branch from 8e91f8f to 2c67fe8 Compare November 8, 2023 12:08

yingsu00 reviewed Nov 9, 2023

View reviewed changes

yingsu00 previously approved these changes Nov 9, 2023

View reviewed changes

yingsu00 reviewed Nov 9, 2023

View reviewed changes

yingsu00 approved these changes Nov 9, 2023

View reviewed changes

agrawalreetika approved these changes Nov 13, 2023

View reviewed changes

imjalpreet approved these changes Nov 13, 2023

View reviewed changes

yingsu00 merged commit 4f9bc23 into prestodb:master Nov 14, 2023
51 checks passed

wanglinsong mentioned this pull request Dec 8, 2023

Add release notes for 0.285 #21500

Closed

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix iceberg table creation using glue metastore #20699

Fix iceberg table creation using glue metastore #20699

pratyakshsharma commented Aug 29, 2023

github-actions bot commented Aug 29, 2023 •

edited

Loading

steveburnett left a comment

steveburnett Aug 29, 2023

pratyakshsharma commented Sep 5, 2023

imjalpreet left a comment

imjalpreet Sep 7, 2023

imjalpreet Sep 7, 2023

pratyakshsharma Sep 20, 2023

imjalpreet Sep 7, 2023

pratyakshsharma Sep 20, 2023

imjalpreet Sep 7, 2023

pratyakshsharma commented Sep 20, 2023

yingsu00 left a comment

pratyakshsharma commented Oct 11, 2023

agrawalreetika Oct 30, 2023

imjalpreet Oct 30, 2023

pratyakshsharma Nov 8, 2023

yingsu00 left a comment

yingsu00 left a comment

yingsu00 commented Nov 9, 2023

imjalpreet left a comment

	`GlueHiveMetastore` makes use of DynamoDb based locking mechanism. Please assign a policy similar to the below to the IAM user being used for creating table using glue catalog::
	`GlueHiveMetastore` makes use of DynamoDb based locking mechanism. Please assign a policy similar to the one shown below to the IAM user being used for creating tables using a Glue catalog::

Fix iceberg table creation using glue metastore #20699

Fix iceberg table creation using glue metastore #20699

Conversation

pratyakshsharma commented Aug 29, 2023

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

github-actions bot commented Aug 29, 2023 • edited Loading

steveburnett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pratyakshsharma commented Sep 5, 2023

imjalpreet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pratyakshsharma commented Sep 20, 2023

yingsu00 left a comment

Choose a reason for hiding this comment

pratyakshsharma commented Oct 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yingsu00 left a comment

Choose a reason for hiding this comment

yingsu00 left a comment

Choose a reason for hiding this comment

yingsu00 commented Nov 9, 2023

imjalpreet left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 29, 2023 •

edited

Loading