vladson commented on code in PR #1894:
URL: https://github.com/apache/iceberg-rust/pull/1894#discussion_r2647968601
##########
crates/catalog/hms/src/catalog.rs:
##########
@@ -603,10 +705,94 @@ impl Catalog for HmsCatalog {
))
}
- async fn update_table(&self, _commit: TableCommit) -> Result<Table> {
- Err(Error::new(
- ErrorKind::FeatureUnsupported,
- "Updating a table is not supported yet",
- ))
+ /// Updates an existing table by applying a commit operation.
+ ///
+ /// This method supports two update strategies depending on the catalog
configuration:
+ ///
+ /// **Optimistic Locking** (when `hive_locks_disabled` is set):
+ /// - Retrieves the current table state from HMS without acquiring locks
+ /// - Constructs an `EnvironmentContext` with the expected metadata
location
+ /// - Uses `alter_table_with_environment_context` to perform an atomic
+ /// compare-and-swap operation.
+ /// - HMS will reject the update if the metadata location has changed,
+ /// indicating a concurrent modification
+ ///
+ /// **Traditional Locking** (default):
+ /// - Acquires an exclusive HMS lock on the table before making changes
+ /// - Retrieves the current table state
+ /// - Applies the commit and writes new metadata
+ /// - Updates the table in HMS using `alter_table`
+ /// - Releases the lock after the operation completes
+ ///
+ /// # Returns
+ /// A `Result` wrapping the updated `Table` object with new metadata.
+ ///
+ /// # Errors
+ /// This function may return an error in several scenarios:
+ /// - Failure to validate the namespace or table identifier
+ /// - Inability to acquire a lock (traditional locking mode)
+ /// - Failure to retrieve the table from HMS
+ /// - Errors reading or writing table metadata
+ /// - HMS rejects the update due to concurrent modification (optimistic
locking)
+ /// - Errors from the underlying Thrift communication with HMS
+ async fn update_table(&self, commit: TableCommit) -> Result<Table> {
+ let ident = commit.identifier().clone();
+ let db_name = validate_namespace(ident.namespace())?;
+ let tbl_name = ident.name.clone();
+
+ if self.config.props.contains_key(HMS_HIVE_LOCKS_DISABLED) {
Review Comment:
Hey, I had some experience with Hive metastore, both in setting up and using
it. The thing is that many are using optimistic locking, and the problem is
that in a system with multiple clients, all must use no hive locks flow, as
otherwise there is a risk of data loss.
My implementation of the environment context is in line [with
Trino](https://github.com/trinodb/trino/blob/be3fd107448c652be43e72c3e8bfef5bcf83a32f/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/HiveMetastoreTableOperations.java#L143)
and others.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
