Repository navigation
Conversation
…cation CAS
- Without a lock (HIVE-28851), HiveIcebergMetaHook refreshed metadata_location before alter_table, but HMS wrote it
unconditionally, so an Iceberg commit landing in between was reverted (e.g. INSERT's stats update).
- Send the refreshed location as the expected value (HiveOperationsBase.hmsEnvContext); HMS rejects the alter instead.
…UID only on a manual change - ColStatsProcessor: a rejected COLUMN_STATS_ACCURATE update logs a warning instead of failing the committed write, unless hive.stats.reliable - HiveIcebergMetaHook: read the caller's metadata.json for the UUID check only on a manual metadata_location change - Tests: concurrent insert during the column stats update; alter with an expired caller metadata_location
…d the HMS lock - HiveIcebergMetaHook: an alter without an operation type (statistics updates, incl. ANALYZE) takes NoLock and the metadata_location CAS, replacing the HiveOperation.QUERY check - It reads the current metadata_location from HMS, as HiveTableOperations does, instead of loading the Iceberg table - Catalogs.hiveTableOps: HiveCatalog table operations without loading metadata - Fixup and CAS shared in keepCurrentMetadataLocation, gated on HiveOperationsBase - Tests: no metadata file read; CAS with Hive locking enabled; expired caller metadata on a property alter
- Catalogs.hiveTableOps: empty unless the table's catalog is a HiveCatalog; an alter without an operation type falls back to the full load otherwise (a catalog-impl HiveCatalog kept the stale metadata_location) - Tests: alter on a catalog-impl HiveCatalog; race tests pinned to one format, stub cleared after the insert - ColStatsProcessor: comment also fits ANALYZE FOR COLUMNS
…akes the commits' lock - MetastoreLock: lock and find the lock in the table's HMS catalog; the Iceberg catalog name only qualifies messages and the JVM lock (since HIVE-29244 locks of different Iceberg catalogs never conflicted) - HiveTableOperations.metastoreLock: the lock the table's commits take - HiveIcebergMetaHook: takes that lock instead of one named after the table identifier; NoLock outside a HiveCatalog - Test: the meta hook's lock excludes a commit through another Iceberg catalog
deniskuzZ
marked this pull request as draft
October 29, 2026 17:29
- HiveIcebergMetaHook: an alter without an operation type reads metadata_location through the hook's client pool and leaves a table without it alone; only an alter that commits to Iceberg and writes the pointer to HMS takes the lock (none with SKIP_METASTORE_ALTER), named after the Iceberg catalog; CAS keys via hive_metastoreConstants - MetastoreLock: no catalog on the HMS lock and its lookup, so every engine's lock of the table conflicts; Catalogs and HiveTableOperations back to master - Tests: another engine's lock per alter kind; a table without metadata_location; the no-read check merged into testMetaHookWithUndefinedAlterOperationType; renames
asf-ci-hive
added
tests passed
tests pending
and removed
tests pending
tests passed
labels
- MetastoreLock back to master: the HMS lock carries the Iceberg catalog name (HIVE-29244); the hook's lock uses the table's catalog name (Catalogs.catalogName) instead of the table identifier - Catalogs.hiveCatalog: a HiveCatalog configured by catalog-impl counts - HiveIcebergMetaHook: CAS instead of the lock under external locking; the manual metadata_location check and the fixup branch once, sharing expectMetadataLocation - Tests: the alter callback moves into HiveIcebergStorageHandlerStub; lock tests take the table's catalog; external locking and catalog-impl cases
deniskuzZ
changed the title
|
- External locking requires engine.hive.lock-enabled=false (getLockType rejects writes otherwise), where the hook already takes no lock
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
HiveIcebergMetaHookprotects the HMSalter_tableit wraps the wayHiveTableOperations.doCommitprotects an Iceberg commit: either the HMS lock or a compare-and-swap (CAS) onmetadata_location(expected_parameter_key/expected_parameter_value).metadata_locationfrom HMS, asdoCommitdoes, puts it on the outgoing table (the HIVE-26576 fixup) and adds the CAS. HMS then rejects the alter if a commit moved the pointer in the meantime.metadata_locationto HMS take the lock Hive's Iceberg commits take, named after the table's Iceberg catalog (the hook passed the table identifier before). An alter that makes no HMS write (SKIP_METASTORE_ALTER), or a table withoutmetadata_location, gets no lock.metadata.jsononly for a manualSET TBLPROPERTIES('metadata_location'=...). Every other alter takes the current location, so a stats alter no longer readsmetadata.jsonand no longer fails when the caller's metadata file has expired.ColStatsProcessor: if HMS rejects theCOLUMN_STATS_ACCURATEalter, it logs a warning instead of failing a statement whose data and statistics are already committed. Withhive.stats.reliableit still fails, asBasicStatsTaskdoes.Catalogs.hiveCatalogalso recognizes a HiveCatalog configured bycatalog-impl.Why are the changes needed?
HIVE-28851 made the hook take no lock when
engine.hive.lock-enabled=falseand for everyHiveOperation.QUERY, without adding a CAS. The hook refreshesmetadata_locationinto the outgoing table, butalter_tablereplaces all table parameters. So a commit by another writer (Impala, Spark, another Hive session) that lands between the refresh and the HMS write gets reverted:metadata_locationgoes back to the old file and the new snapshot is lost.Reproduced with Impala: the INSERT's stats alter paused after the hook, Impala committed
00003->00004, and the alter then wrote00003back. Also reproduced with a second Hive session.Does this PR introduce any user-facing change?
Yes, when an alter races with a concurrent commit:
hive.stats.reliable=true. Before, it reverted the other commit.metastore.try.direct.sql=true, the default), the same requirement as Iceberg commits withengine.hive.lock-enabled=false.useAffectedRows=true, an UPDATE that rewrites the same value reports 0 rows, so HMS rejects a stats alter even when no commit raced with it.How was this patch tested?
TestHiveIcebergStatistics#testStatsUpdateDoesNotRevertConcurrentInsertand#testColumnStatsUpdateDoesNotFailOnConcurrentInsertrun a second Hive session's INSERT between the hook and the HMS call. Both inserts survive and the statement succeeds.TestHiveIcebergStorageHandlerNoScancovers:metadata.jsonread, the current pointer plus the CAS;catalog-impl;metadata_location;TestHiveCatalog,TestHiveCommitLocks,TestHiveCommits,TestHiveTableConcurrency,TestHiveViewCatalog,TestHiveViewCommits) and the iceberg-handler suites (TestHiveIcebergStatistics,TestHiveIcebergStorageHandlerNoScan,TestHiveIcebergMigration,TestHiveIcebergSetCurrentSnapshot,TestConflictingDataFiles,TestOptimisticRetry,TestHiveIcebergWriteMetadataCleanup,TestConcurrentMultiTableTransactions) pass: 1355 tests.