pvary commented on code in PR #12637:
URL: https://github.com/apache/iceberg/pull/12637#discussion_r2012230419
##########
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;
+ }
+
+ /**
+ * Attempt to load the content and see if any current or past metadata
location matches the one we
+ * were attempting to set.
+ *
+ * @param tableOrViewName full name of the Table/View
+ * @param newMetadataLocation the path of the new commit file
+ * @param properties properties for retry
+ * @param commitStatusSupplier check if the latest metadata presents or not
using metadata
+ * location for table.
+ * @return Empty if locations cannot be checked, e.g. unable to refresh.
True if the new location
+ * is committed, false otherwise.
+ */
+ protected Optional<Boolean> metadataLocationCommitted(
Review Comment:
Instead of returning an `Optional<Boolean>`, would it be better to return
the current commit status?
Like:
```
/**
* Attempt to load the content and see if any current or past metadata
location matches the one we
* were attempting to set. This is used as a last resort when we are
dealing with exceptions that
* may indicate the commit has failed and don't have proof that this is
the case, but we can be sure that
* no retry attempts for the commit will be successful later. Note that
all the previous locations must also
* be searched on the chance that a second committer was able to
successfully commit on top of our commit.
* When the {@code newMetadataLocation} is not in the history the method
returns
* {@link CommitStatus.FAILURE}, when the {@link commitStatusSupplier}
fails repeatedly the method returns
* {@link CommitStatus.UNKNOWN}.
*
* @param tableOrViewName full name of the Table/View
* @param newMetadataLocation the path of the new commit file
* @param properties properties for retry
* @param commitStatusSupplier check if the latest metadata presents or
not using metadata
* location for table.
* @return Commit Status of Success, Failure or Unknown
*/
protected CommitStatus checkCommitStatusStrict(
String tableOrViewName,
String newMetadataLocation,
Map<String, String> properties,
Supplier<Boolean> commitStatusSupplier) {
```
And we need to update the javadoc for the `checkCommitStatus` too, to
describe that it will never return `CommitStatus.FAILURE`, like:
```
/**
* Attempt to load the content and see if any current or past metadata
location matches the one we
* were attempting to set. This is used as a last resort when we are
dealing with exceptions that
* may indicate the commit has failed but don't have proof that this is
the case. Note that all
* the previous locations must also be searched on the chance that a
second committer was able to
* successfully commit on top of our commit. When the {@code
newMetadataLocation} is not in the
* history or the {@link commitStatusSupplier} fails repeatedly the method
returns
* {@link CommitStatus.UNKNOWN}, because possible pending retires might
still commit the change.
*
* @param tableOrViewName full name of the Table/View
* @param newMetadataLocation the path of the new commit file
* @param properties properties for retry
* @param commitStatusSupplier check if the latest metadata presents or
not using metadata
* location for table.
* @return Commit Status of Success or Unknown
*/
```
--
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]
