deniskuzZ commented on code in PR #6930:
URL: https://github.com/apache/hive/pull/6930#discussion_r4469198986
##########
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:
Good catch, the catalog on the lock was the root cause: HIVE-29244 put the
Iceberg catalog name on the HMS lock, while other engines lock without a
catalog. HIVE-30231 reverted that part of HIVE-29244 (merged), so Hive's commit
lock and the hook's lock now conflict with every other engine's. With that in
place I'd keep lock or CAS, never both, as `HiveTableOperations.doCommit` and
upstream Iceberg do. The stats alters this PR is about always use the CAS,
whatever the lock mode.
--
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]
