pvary commented on a change in pull request #2328:
URL: https://github.com/apache/iceberg/pull/2328#discussion_r600281372
##########
File path:
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
##########
@@ -270,29 +270,33 @@ private CommitStatus checkCommitStatus(String
newMetadataLocation, TableMetadata
int maxAttempts = PropertyUtil.propertyAsInt(config.properties(),
COMMIT_NUM_STATUS_CHECKS,
COMMIT_NUM_STATUS_CHECKS_DEFAULT);
- for (int attempt = 1; attempt <= maxAttempts; attempt++) {
- try {
- Thread.sleep(COMMIT_STATUS_RECHECK_SLEEP);
- TableMetadata metadata = refresh();
- String metadataLocation = metadata.metadataFileLocation();
- boolean commitSuccess = metadataLocation.equals(newMetadataLocation) ||
- metadata.previousFiles().stream().anyMatch(log ->
log.file().equals(newMetadataLocation));
- if (commitSuccess) {
- LOG.info("Commit status check: Commit to {}.{} of {} succeeded",
database, tableName, newMetadataLocation);
- return CommitStatus.SUCCESS;
- } else {
- LOG.info("Commit status check: Commit to {}.{} of {} failed",
database, tableName, newMetadataLocation);
- return CommitStatus.FAILURE;
- }
- } catch (Throwable checkFailure) {
- LOG.error("Cannot check if commit to {}.{} exists. Retry attempt {} of
{}.",
- database, tableName, attempt, maxAttempts, checkFailure);
- }
- }
-
- LOG.error("Cannot determine commit state to {}.{}. Failed to check {}
times. Treating commit state as unknown.",
+ AtomicReference<CommitStatus> status = new
AtomicReference<>(CommitStatus.UNKNOWN);
+
+ Tasks
+ .foreach(newMetadataLocation)
+ .retry(maxAttempts)
+ .suppressFailureWhenFinished()
+ .exponentialBackoff(COMMIT_STATUS_RECHECK_SLEEP,
COMMIT_STATUS_RECHECK_SLEEP, Long.MAX_VALUE, 1.0)
Review comment:
@aokolnychyi: Not sure about the target of the question, so I try to
elaborate more and hope that somewhere I hit the target 😄
- In HiveServer2 we have a HMS client for every session. If we reuse this
client instead of creating a new one for the Iceberg catalogs we can save
resources. Also, as I have discovered lately this HMS client could filter the
databases / tables based on the roles of the current user (this might not be
that important since the first access of the table will go through the HMS
client in the session anyway, so this will prevent unauthorized access, but
still...).
- In Hive we use
[RetryingMetaStoreClient](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/RetryingMetaStoreClient.java)
(moved between 2.3.8 and 3.1.2 but exists in both releases under different
path and configs) which retries the calls several times defined by the
`THRIFT_FAILURE_RETRIES` defaults to 1 after `CLIENT_CONNECT_RETRY_DELAY`
defaults to 1 second. This might be useful replacement for the retrying
mechanism created here, but we have to examine if the retries would cause any
issue on other use-cases of the HiveClientPool. The most important thing is
that we should turn off one of the retry mechanism if we start to use the
`RetryingMetaStoreClient` otherwise we will face some surprises down the road.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
