HIVE-28810: HiveIcebergMetaHook to respect table lock property #5687
Conversation
|
| if (commitLock == null) { | ||
| commitLock = new MetastoreLock(conf, new CachedClientPool(conf, Maps.fromProperties(catalogProperties)), | ||
| catalogProperties.getProperty(Catalogs.NAME), hmsTable.getDbName(), hmsTable.getTableName()); | ||
| if (HiveTableOperations.hiveLockEnabled(null, conf)) { |
There was a problem hiding this comment.
I think, HiveIcebergMetaHook should always use MetastoreLock.
what if Hive conf is set to NO_LOCK, but the user has overridden it on the table level to METASTORE_LOCK? That would break the transaction isolation.
METASTORE_LOCK is a stronger one that checks if metadataLocation hasn't changed.
There was a problem hiding this comment.
what is interesting is that I can almost only see this whole hiveLockEnabled(...) thing used in testing scenarios, which applies to my case as well, see screenshot
this makes me think that if I want to make my query history PR snapshot work then
a) I have to override the locking behavior and bypass it in case of tests by extending the meta hook instead and use a testing one
b) or make it work with metastore locking
There was a problem hiding this comment.
seems like b) works, I'm about to close this one soon
now I would appreciate a review on #5666 to finalize the first phase of query history maybe, thanks in advance!
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
4 participants
What changes were proposed in this pull request?
Commitlock to be created according to HiveTableOperations.hiveLockEnabled.
Why are the changes needed?
Mainly for testing purposes, where iceberg.engine.hive.lock-enabled=false, but anyway, other codepaths respect this setting before creating a lock object.
Does this PR introduce any user-facing change?
No.
Is the change a dependency upgrade?
No.
How was this patch tested?
A test introduced in HIVE-28802 without this change, waiting on a lock, and passed with this fix.