Support iceberg connector concurrent insertion

[liupan664021]

Before this pr, if multiple clients insert into same table concurrently, the insertion can be exexuted successfully but some insertion will lost. Because the snapshot replacement required a metastore lock to make sure the isolation. Also the native HiveTableOperations in iceberg implements the lock guard.

== RELEASE NOTES ==

Iceberg Changes
* Support concurrent insertion from the same presto cluster or mutiple presto clusters which sharing the same metastore

[beinan]

My concern is the lock might not work properly when there are multiple presto clusters or other compute engines who are updating the certain table. I might propose to retry the failed insert ops rather than lock the table in presto. What do you think?

[liupan664021]

My concern is the lock might not work properly when there are multiple presto clusters or other compute engines who are updating the certain table. I might propose to retry the failed insert ops rather than lock the table in presto. What do you think?

@beinan Thanks for the review! Actually there are two level lock to gurantee the doCommit operation: a jvm level lock and a hive metastore lock acquired by a ThriftHiveMetastoreClient#lock. The lock part is similar to the iceberg native HiveTableOperations

[junyi1313]

My concern is the lock might not work properly when there are multiple presto clusters or other compute engines who are updating the certain table. I might propose to retry the failed insert ops rather than lock the table in presto. What do you think?

@beinan Thanks for the review! Actually there are two level lock to gurantee the doCommit operation: a jvm level lock and a hive metastore lock acquired by a ThriftHiveMetastoreClient#lock. The lock part is similar to the iceberg native HiveTableOperations

Add a cherry pick info may be better @liupan664021

[beinan]

My concern is the lock might not work properly when there are multiple presto clusters or other compute engines who are updating the certain table. I might propose to retry the failed insert ops rather than lock the table in presto. What do you think?

@beinan Thanks for the review! Actually there are two level lock to gurantee the doCommit operation: a jvm level lock and a hive metastore lock acquired by a ThriftHiveMetastoreClient#lock. The lock part is similar to the iceberg native HiveTableOperations

Add a cherry pick info may be better @liupan664021

My concern is the lock might not work properly when there are multiple presto clusters or other compute engines who are updating the certain table. I might propose to retry the failed insert ops rather than lock the table in presto. What do you think?

@beinan Thanks for the review! Actually there are two level lock to gurantee the doCommit operation: a jvm level lock and a hive metastore lock acquired by a ThriftHiveMetastoreClient#lock. The lock part is similar to the iceberg native HiveTableOperations

Got it, that makes a lot of sense. Thank you for the explanation!

If it's a cherry-pick, could you link the original commit? thanks!

[liupan664021]

@beinan @junyi1313 Sure, I'll add the cherry-pick info.

[liupan664021]

My concern is the lock might not work properly when there are multiple presto clusters or other compute engines who are updating the certain table. I might propose to retry the failed insert ops rather than lock the table in presto. What do you think?

@beinan I tested concurrent insertions from two different presto cluster, and all the insertions finished without data missing. It seems the hive metastore lock and jvm lock work well. Without the lock, even concurrent insertions from same presto cluster may cause data missing. Any suggestion is appreciated :)

[beinan]

@ChunxuTang could you also help take a look? Thanks!

[liupan664021]

@ChunxuTang could you also help take a look? Thanks!

@ChunxuTang Could you help take a look of this PR? Thanks!

[ChunxuTang]

@liupan664021 @beinan Glad to help! Will review it soon.

[ChunxuTang]

@liupan664021 Thanks for your nice work!
For the commit, as Apache Iceberg and Presto are two different projects, I think it's not a cherry-pick commit, though you reuse some functionalities from there. I would suggest just linking the two Iceberg commits in your description of the PR.

[ChunxuTang]

@beinan BTW, just for a double check: I think this feature works for the HiveMetastore mode but not the native folder mode, right?

[liupan664021]

@liupan664021 Thanks for your nice work! For the commit, as Apache Iceberg and Presto are two different projects, I think it's not a cherry-pick commit, though you reuse some functionalities from there. I would suggest just linking the two Iceberg commits in your description of the PR.

Thanks for the suggestion! I've updated the commit message and rebased the branch.

[liupan664021]

@beinan @ChunxuTang Could you help have a look of this PR, thanks!

[liupan664021]

@beinan Could you help to merge this PR? Any comments would be appreciated~ Thanks a lot :)

[beinan]

Sorry for my delay. it looks good to me except a couple of minor style issues.

@ChunxuTang @zhenxiao do you to wanna a second look?

[zhenxiao]

thank you, @liupan664021
looks good to me
could you please confirm:

• this PR only references iceberg code changes. it is not a cherry-pick from trino or other projects
• with locking, multiple Presto clusters sharing the same metatore could support concurrent insert across Presto clusters, and if so, please add documentations for it, either in commit message, or the release notes

[beinan]

thank you, @liupan664021 looks good to me could you please confirm:

• this PR only references iceberg code changes. it is not a cherry-pick from trino or other projects
• with locking, multiple Presto clusters sharing the same metatore could support concurrent insert across Presto clusters, and if so, please add documentations for it, either in commit message, or the release notes

Hello @liupan664021, could you address Zhenxiao's comments? since this pr is super useful and has been there for a while, I would like to merge it sooner rather than later. Thanks a lot!

[liupan664021]

thank you, @liupan664021 looks good to me could you please confirm:

• this PR only references iceberg code changes. it is not a cherry-pick from trino or other projects
• with locking, multiple Presto clusters sharing the same metatore could support concurrent insert across Presto clusters, and if so, please add documentations for it, either in commit message, or the release notes

Hello @liupan664021, could you address Zhenxiao's comments? since this pr is super useful and has been there for a while, I would like to merge it sooner rather than later. Thanks a lot!

Thanks for your replies! @zhenxiao @beinan

1. This PR only references the iceberg code changes, not a cherry-pick from any other project including Trino.
2. I tested concurrent insertions from two presto cluster which sharing the same metastore, and it works well. Also, I added the infomation to the Release notes.

[abhiseksaikia]

@liupan664021, @beinan This change is breaking our builds due to new methods added to HiveMetastore interface. Could you please add default implementation if that makes sense or revert this PR.

[beinan]

@abhiseksaikia I will try to add a default implementation asap.

[pratyakshsharma]

@beinan @liupan664021 This fix does not seem to work for more than 5 concurrent queries fired at the same time for iceberg connector. Can you please highlight how did you guys test this and if there is any configuration tuning needed to support more than 5 concurrent queries?
cc @tdcmeehan