Add support for lock-free commit in Iceberg HMS catalog#25445
Conversation
There was a problem hiding this comment.
I'a take that asserting no lock has been acquired is unfeasible/impossible
There was a problem hiding this comment.
I'm not sure this is necessary, but if you fetch /tmp/root/hive.log from the metastore container and look for lines with HiveMetaStore.audit, you should see all of the metastore Thrift calls.
electrum
left a comment
There was a problem hiding this comment.
How does this work? Based on my reading of the HMS change, it seems like we need to pass the “expectedParameter” value to the metastore so that it can do the check.
1074ef8 to
da96ac3
Compare
da96ac3 to
21a9e9c
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds configurable support for lock-free commits in the Hive catalog when using Iceberg. It introduces a new lockingEnabled configuration option in the Iceberg Hive catalog, integrates optional Hive lock acquisition in commit operations, and updates method signatures across metastore interfaces with a new environmentContext parameter.
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/HiveMetastoreTableOperationsProvider.java | Loads and propagates the new lockingEnabled configuration. |
| plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/HiveMetastoreTableOperations.java | Integrates conditional Hive lock acquisition and release in table commit operations. |
| plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/file/FileMetastoreTableOperations.java | Updates the replaceTable call to include an empty environment context. |
| plugin/trino-hive/* | Updates the method signatures of replaceTable and alterTable to accept an environmentContext parameter. |
| docs/src/main/sphinx/object-storage/metastores.md | Documents the new Iceberg Hive catalog property for controlling locking. |
Comments suppressed due to low confidence (1)
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/HiveMetastoreTableOperations.java:95
- [nitpick] Consider renaming the local variable 'lockingEnabled' (e.g., to 'tableLockingEnabled') to clearly distinguish it from the class member.
boolean lockingEnabled = parseBoolean(table.getParameters().getOrDefault(HIVE_LOCK_ENABLED, Boolean.toString(this.lockingEnabled)));
21a9e9c to
4570e2e
Compare
There was a problem hiding this comment.
I'm not sure this is necessary, but if you fetch /tmp/root/hive.log from the metastore container and look for lines with HiveMetaStore.audit, you should see all of the metastore Thrift calls.
4570e2e to
70fcab3
Compare
Description
Fixes #22182
Request from Iceberg community: https://apache-iceberg.slack.com/archives/C025PH0G1D4/p1743095891453949
Iceberg documentation: https://iceberg.apache.org/docs/nightly/configuration/#hadoop-configuration
Release notes