difin commented on code in PR #6930:
URL: https://github.com/apache/hive/pull/6930#discussion_r4428805744
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java:
##########
@@ -399,39 +411,64 @@ private void
doPreAlterTable(org.apache.hadoop.hive.metastore.api.Table hmsTable
}
/**
- * Perform a check on the current iceberg table whether a metadata change
can be performed. A table is eligible if
- * the current metadata uuid and the new metadata uuid matches.
+ * Perform a check on the current iceberg table whether a metadata change
can be performed. A manual change is
+ * eligible if the current metadata uuid and the new metadata uuid matches;
any other alter keeps the current
+ * metadata location.
* @param tblParams hms table properties, must be non-null
*/
private void assertNotCrossTableMetadataLocationChange(Map<String, String>
tblParams, EnvironmentContext context) {
if
(tblParams.containsKey(BaseMetastoreTableOperations.METADATA_LOCATION_PROP)) {
Preconditions.checkArgument(icebergTable != null,
"Cannot perform table migration to Iceberg and setting the snapshot
location in one step. " +
"Please migrate the table first");
- String newMetadataLocation =
tblParams.get(BaseMetastoreTableOperations.METADATA_LOCATION_PROP);
- FileIO io = ((BaseTable) icebergTable).operations().io();
- TableMetadata newMetadata = TableMetadataParser.read(io,
newMetadataLocation);
TableMetadata currentMetadata = ((BaseTable)
icebergTable).operations().current();
- if (!currentMetadata.uuid().equals(newMetadata.uuid())) {
- throw new UnsupportedOperationException(
- String.format("Cannot change iceberg table %s metadata location
pointing to another table's metadata %s",
- icebergTable.name(), newMetadataLocation)
- );
- }
- if (!currentMetadata.metadataFileLocation().equals(newMetadataLocation)
&&
-
!context.getProperties().containsKey(MANUAL_ICEBERG_METADATA_LOCATION_CHANGE)) {
- // If we got here then this is an alter table operation where the
table to be changed had an Iceberg commit
- // meanwhile. The base metadata locations differ, while we know that
this wasn't an intentional, manual
- // metadata_location set by a user. To protect the interim commit we
need to refresh the metadata file location.
- tblParams.put(BaseMetastoreTableOperations.METADATA_LOCATION_PROP,
currentMetadata.metadataFileLocation());
- LOG.warn("Detected an alter table operation attempting to do
alterations on an Iceberg table with a stale " +
- "metadata_location. Considered base metadata_location: {}, Actual
metadata_location: {}. Will override " +
- "this request with the refreshed metadata_location in order to
preserve the concurrent commit.",
- newMetadataLocation, currentMetadata.metadataFileLocation());
+ if
(context.getProperties().containsKey(MANUAL_ICEBERG_METADATA_LOCATION_CHANGE)) {
+ String newMetadataLocation =
tblParams.get(BaseMetastoreTableOperations.METADATA_LOCATION_PROP);
+ FileIO io = ((BaseTable) icebergTable).operations().io();
+ TableMetadata newMetadata = TableMetadataParser.read(io,
newMetadataLocation);
+ if (!currentMetadata.uuid().equals(newMetadata.uuid())) {
+ throw new UnsupportedOperationException(
+ String.format("Cannot change iceberg table %s metadata location
pointing to another table's metadata %s",
+ icebergTable.name(), newMetadataLocation)
+ );
+ }
+ expectMetadataLocation(currentMetadata.metadataFileLocation(),
context);
+ } else {
+ keepCurrentMetadataLocation(tblParams,
currentMetadata.metadataFileLocation(), context);
}
}
}
+ /**
+ * Makes the alter carry the current metadata location.
+ */
+ private void keepCurrentMetadataLocation(Map<String, String> tblParams,
String currentMetadataLocation,
+ EnvironmentContext context) {
+ String metadataLocation =
tblParams.get(BaseMetastoreTableOperations.METADATA_LOCATION_PROP);
+ if (!currentMetadataLocation.equals(metadataLocation)) {
+ // If we got here then this is an alter table operation where the table
to be changed had an Iceberg commit
+ // meanwhile. The base metadata locations differ, while we know that
this wasn't an intentional, manual
+ // metadata_location set by a user. To protect the interim commit we
need to refresh the metadata file location.
+ tblParams.put(BaseMetastoreTableOperations.METADATA_LOCATION_PROP,
currentMetadataLocation);
+ LOG.warn("Detected an alter table operation attempting to do alterations
on an Iceberg table with a stale " +
+ "metadata_location. Considered base metadata_location: {}, Actual
metadata_location: {}. Will override " +
+ "this request with the refreshed metadata_location in order to
preserve the concurrent commit.",
+ metadataLocation, currentMetadataLocation);
+ }
+ expectMetadataLocation(currentMetadataLocation, context);
+ }
+
+ /**
+ * Without a lock, HMS applies the alter only if no commit moved the
metadata location since it was read.
+ */
+ private void expectMetadataLocation(String currentMetadataLocation,
EnvironmentContext context) {
+ if (commitLock instanceof NoLock) {
Review Comment:
Skipping the CAS under `MetastoreLock` is safe only if the lock excludes
every other writer of `metadata_location`, and it doesn't. `MetastoreLock` sets
the lock's catalog to the Iceberg catalog name
(l`ockComponent.setCatName(catalogName)`, from `Catalogs.catalogName(conf,
tableProperties)` in `lockObject`). `CheckLockFunction` only treats two locks
as conflicting when `HL_CATALOG` matches.
So this lock blocks only Hive writers that use the same Iceberg catalog
name. It doesn't block Hive sessions that reach the same table under a
different catalog name. It also may not block Spark, Trino or Impala if their
locks carry a different catalog value. Those writers also skip the CAS while
holding their lock.
As a result, `ALTER TABLE ... SET TBLPROPERTIES `on a `lock-enabled` table,
including a manual `metadata_location` change, can still overwrite a concurrent
commit's pointer. That's the data loss this PR fixes for the unlocked path.
Could we always send the CAS, even under the lock? It costs one conditional
UPDATE and covers writers the lock can't see. `HiveTableOperations.doCommit`
has the same gap (`hiveLockEnabled ? null : baseMetadataLocation)`, but that
predates this PR and could be a follow-up. A test with a concurrent commit
under a different catalog name would cover this.
--
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]
