lirui-apache commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2011775085
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -287,15 +277,25 @@ protected void doCommit(TableMetadata base, TableMetadata
metadata) {
e);
}
- LOG.error(
- "Cannot tell if commit to {}.{} succeeded, attempting to reconnect
and check.",
- database,
- tableName,
- e);
- commitStatus = BaseMetastoreOperations.CommitStatus.UNKNOWN;
- commitStatus =
- BaseMetastoreOperations.CommitStatus.valueOf(
- checkCommitStatus(newMetadataLocation, metadata).name());
+ if (e.getMessage() != null
+ && e.getMessage()
Review Comment:
Good catch, will do
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreOperations.java:
##########
@@ -63,6 +64,31 @@ protected CommitStatus checkCommitStatus(
String newMetadataLocation,
Map<String, String> properties,
Supplier<Boolean> commitStatusSupplier) {
+ if (metadataLocationCommitted(
+ tableOrViewName, newMetadataLocation, properties,
commitStatusSupplier)
+ .orElse(false)) {
+ return CommitStatus.SUCCESS;
+ }
+ return CommitStatus.UNKNOWN;
Review Comment:
Because `checkCommitStatus` never returns `CommitStatus.FAILURE`, which is
required so that we know when to throw a CommitFailedException. And since it's
kind of a public API, so I added metadataLocationCommitted which returns false
if the new metadata location cannot be found.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -324,6 +324,25 @@ protected void doCommit(TableMetadata base, TableMetadata
metadata) {
"Committed to table {} with the new metadata location {}", fullName,
newMetadataLocation);
}
+ private BaseMetastoreOperations.CommitStatus handleConcurrentModification(
+ Throwable throwable, String newMetadataLocation, TableMetadata metadata)
{
Review Comment:
Same here as in the previous comment: `checkCommitStatus` doesn't return
FAILURE which is required by `handleConcurrentModification `
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
