difin commented on code in PR #6930:
URL: https://github.com/apache/hive/pull/6930#discussion_r4428784556
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java:
##########
@@ -307,24 +305,38 @@ public void
preAlterTable(org.apache.hadoop.hive.metastore.api.Table hmsTable, E
}
}
- private HiveLock lockObject(org.apache.hadoop.hive.metastore.api.Table
hmsTable) {
- if (!HiveTableOperations.hiveLockEnabled(hmsTable.getParameters(), conf) ||
- SessionStateUtil.getQueryState(conf)
- .map(QueryState::getHiveOperation)
- .filter(opType -> HiveOperation.QUERY == opType)
- .isPresent()) {
+ /**
+ * The HMS lock Hive's Iceberg commits take, for an alter that commits to
Iceberg and writes the metadata_location to
+ * HMS. An alter without an operation type commits nothing to Iceberg: the
metadata_location CAS protects it instead.
+ */
+ private HiveLock lockObject(org.apache.hadoop.hive.metastore.api.Table
hmsTable, EnvironmentContext context) {
+ Map<String, String> params = hmsTable.getParameters();
+ boolean writesMetadataLocation = currentAlterTableOp != null &&
+
!Boolean.parseBoolean(context.getProperties().get(SKIP_METASTORE_ALTER)) &&
+
params.containsKey(BaseMetastoreTableOperations.METADATA_LOCATION_PROP);
+ if (!writesMetadataLocation ||
!HiveTableOperations.hiveLockEnabled(params, conf)) {
return new NoLock();
- } else {
- return new MetastoreLock(
- conf,
- new CachedClientPool(conf, Maps.fromProperties(tableProperties)),
- tableProperties.getProperty(Catalogs.NAME), hmsTable.getDbName(),
- hmsTable.getTableName());
}
+ return new MetastoreLock(
+ conf,
+ new CachedClientPool(conf, Maps.fromProperties(tableProperties)),
+ Catalogs.catalogName(conf, tableProperties),
+ hmsTable.getDbName(),
+ hmsTable.getTableName());
}
private void doPreAlterTable(org.apache.hadoop.hive.metastore.api.Table
hmsTable, EnvironmentContext context)
- throws MetaException {
+ throws TException, InterruptedException {
+ if (currentAlterTableOp == null) {
+ // nothing to commit to Iceberg: read the current metadata_location from
HMS, as HiveTableOperations does
+ if
(hmsTable.getParameters().containsKey(BaseMetastoreTableOperations.METADATA_LOCATION_PROP))
{
+ String currentMetadataLocation = new CachedClientPool(conf,
Maps.fromProperties(tableProperties))
+ .run(client -> client.getTable(hmsTable.getDbName(),
hmsTable.getTableName()))
+
.getParameters().get(BaseMetastoreTableOperations.METADATA_LOCATION_PROP);
+ keepCurrentMetadataLocation(hmsTable.getParameters(),
currentMetadataLocation, context);
+ }
+ return;
+ }
Review Comment:
For alters with no operation type (basic stats, column stats state, TOUCH),
the CAS now makes a concurrent Iceberg commit cause a rejection. That rejection
reaches the statement when `hive.stats.reliable=true`
(`BasicStatsTask.aggregateStats` and the rethrow in `ColStatsProcessor`).
For a single-table autocommit INSERT, UPDATE, DELETE or MERGE, the snapshot
is already published in the move task (`HiveIcebergOutputCommitter.commit` →
`txn.commitTransaction()`), before the stats task runs. The driver's rollback
doesn't revert it. The user then sees a failed statement whose rows are already
in the table, and a retry duplicates them. Explicit transactions and
multi-table inserts aren't affected, because `HiveTxnCoordinator` stages the
commit until the Hive transaction commits.
These alters commit nothing to Iceberg, so a rejection here can't be a real
conflict. Could we retry them a few times, re-reading metadata_location each
time, instead of failing? Either in the hook or around `db.alterTable` in the
stats tasks. A test with a concurrent commit and `hive.stats.reliable=true`
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]
