deniskuzZ commented on code in PR #6930:
URL: https://github.com/apache/hive/pull/6930#discussion_r4469192589
##########
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:
Thanks, the failure after the commit is real. I'd keep the rejection rather
than retry, though. The hook reads the expected `metadata_location` from HMS
after the statement's own partition-stats and Puffin commits, so the statement
can't reject itself. A rejection means another commit landed between that read
and the UPDATE. The stats being written were computed before that commit, and
the commit cleared `COLUMN_STATS_ACCURATE`. A retry would write them back as
accurate over a newer snapshot. The rejection is the one point where Hive can
tell, and `hive.stats.reliable` exists to fail there; it already fails after
the data is visible for other post-commit stats failures. Without it, the
rejection is a warning, and `ColStatsProcessor` now behaves like
`BasicStatsTask`.
--
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]
