HIVE-26882: Allow transactional check of Table parameter before altering the Table

[pvary]

What changes were proposed in this pull request?

Adding the possibility to transactionally check if a Table parameter is changed before altering the table in the HMS.

Why are the changes needed?

This would provide an alternative, less error-prone and faster way to commit an Iceberg table, as the Iceberg table currently needs to:

• Create an exclusive lock
• Get the table metadata to check if the current snapshot is not changed
• Update the table metadata
• Release the lock

After the change these 4 HMS calls could be substituted with a single alter table call.
Also we could avoid cases where the locks are left hanging by failed processes

Does this PR introduce any user-facing change?

HMS API changes:

• Added expectedParameterKey, and expectedParameterValue to the AlterTableRequest object
• Alternative option to provide hive_metastoreConstants.EXPECTED_PARAMETER_KEY and hive_metastoreConstants.EXPECTED_PARAMETER_VALUE through the EnvironmentContext

How was this patch tested?

Added 2 new unit tests

[szehon-ho]

Did you consider allowing a list of table properties here?

[pvary]

Iceberg does not explicitly need this, and it would somewhat complicate the code. So I voted down this. Still I do not feel too strongly either way, so if you think it could be useful, I am open to add multiple checks based on a map (do we have map in thrift?)

[pvary]

After some more thought, I would opt for keeping the PR as it is - checking only for a single property.
If someone would need more properties to check, then they still could create an uber property - as a hash of the relevant properties -, and use the EXPECTED_PARAMETER_KEY and EXPECTED_PARAMETER_VALUE to check for the changes of this uber property. And if it is that hard for them, they could still add a new feature later.

[szehon-ho]

Though I still think its cleaner to support multi-properties transactional check, given Table has properties map, I'm leave it to you and @prasanthj

[szehon-ho]

For my understanding, this only works if a user has table lock while calling alter right?

If user does not have lock, Hive has no internal lock to prevent two users from both getting their expected value and proceeding right?

[pvary]

Thankfully this is not true.
HMS does these changes using the underlying DB’s transaction.
See:

• hive/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java

  Line 180 in 84e6553

  msdb.openTransaction();

• hive/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java

  Line 558 in 84e6553

  public boolean openTransaction() {

[pvary]

It turns out, that you have been right. We use READ_COMMITTED isolation level for our commits.
Because of this, we could commit concurrently, and lose data.

I had to set the isolation level for these transactions to REPEATABLE_READ explicitly.

@szehon-ho and @prasanthj could you please check it if you are back from holidays?

Thanks,
Peter

[szehon-ho]

Thanks for checking. Is this a setting user can configure in Hive? If so, it sounds like we should document it as a pre-requisite for user to enable this feature

[pvary]

Users does not have to do anything. We set the isolation level per transaction automatically from the code. Similarly as we do in the TxnHandler methods where the default isolation level is not enough.

[pvary]

This is the relevant code part:

msdb.openTransaction(Constants.TX_REPEATABLE_READ)

[szehon-ho]

Sounds good.

[pvary]

Pushed to master.
Thanks @prasanthj and @szehon-ho for the review!