nastra commented on code in PR #10001:
URL: https://github.com/apache/iceberg/pull/10001#discussion_r1530138582
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##########
@@ -309,65 +304,39 @@ protected enum CommitStatus {
* @return Commit Status of Success, Failure or Unknown
*/
protected CommitStatus checkCommitStatus(String newMetadataLocation,
TableMetadata config) {
- int maxAttempts =
- PropertyUtil.propertyAsInt(
- config.properties(), COMMIT_NUM_STATUS_CHECKS,
COMMIT_NUM_STATUS_CHECKS_DEFAULT);
- long minWaitMs =
- PropertyUtil.propertyAsLong(
- config.properties(),
- COMMIT_STATUS_CHECKS_MIN_WAIT_MS,
- COMMIT_STATUS_CHECKS_MIN_WAIT_MS_DEFAULT);
- long maxWaitMs =
- PropertyUtil.propertyAsLong(
- config.properties(),
- COMMIT_STATUS_CHECKS_MAX_WAIT_MS,
- COMMIT_STATUS_CHECKS_MAX_WAIT_MS_DEFAULT);
- long totalRetryMs =
- PropertyUtil.propertyAsLong(
- config.properties(),
- COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS,
- COMMIT_STATUS_CHECKS_TOTAL_WAIT_MS_DEFAULT);
-
- AtomicReference<CommitStatus> status = new
AtomicReference<>(CommitStatus.UNKNOWN);
-
- Tasks.foreach(newMetadataLocation)
- .retry(maxAttempts)
- .suppressFailureWhenFinished()
- .exponentialBackoff(minWaitMs, maxWaitMs, totalRetryMs, 2.0)
- .onFailure(
- (location, checkException) ->
- LOG.error("Cannot check if commit to {} exists.", tableName(),
checkException))
- .run(
- location -> {
- TableMetadata metadata = refresh();
- String currentMetadataFileLocation =
metadata.metadataFileLocation();
- boolean commitSuccess =
- currentMetadataFileLocation.equals(newMetadataLocation)
- || metadata.previousFiles().stream()
- .anyMatch(log ->
log.file().equals(newMetadataLocation));
- if (commitSuccess) {
- LOG.info(
- "Commit status check: Commit to {} of {} succeeded",
- tableName(),
- newMetadataLocation);
- status.set(CommitStatus.SUCCESS);
- } else {
- LOG.warn(
- "Commit status check: Commit to {} of {} unknown, new
metadata location is not current "
- + "or in history",
- tableName(),
- newMetadataLocation);
- }
- });
-
- if (status.get() == CommitStatus.UNKNOWN) {
- LOG.error(
- "Cannot determine commit state to {}. Failed during checking {}
times. "
- + "Treating commit state as unknown.",
- tableName(),
- maxAttempts);
+ return CommitStatus.valueOf(
+ checkCommitStatus(
+ tableName(),
+ newMetadataLocation,
+ config.properties(),
+ () -> checkCurrentMetadataLocation(newMetadataLocation))
+ .name());
+ }
+
+ /**
+ * Checks the new metadata location presents or not after refreshing the
table.
+ *
+ * @param newMetadataLocation newly written metadata location
+ * @return true if the new metadata location presents with current or
previous metadata files.
+ */
+ protected boolean checkCurrentMetadataLocation(String newMetadataLocation) {
+ TableMetadata metadata = refresh();
+ Preconditions.checkNotNull(metadata, "Unexpected null table metadata");
Review Comment:
I would have expected this code to be what the previous code was:
```
TableMetadata metadata = refresh();
String currentMetadataFileLocation = metadata.metadataFileLocation();
boolean commitSuccess =
currentMetadataFileLocation.equals(newMetadataLocation)
|| metadata.previousFiles().stream().anyMatch(log ->
log.file().equals(newMetadataLocation));
```
can you elaborate why the code here now ends up being different?
--
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]
