hudi-agent commented on code in PR #20129:
URL: https://github.com/apache/hudi/pull/20129#discussion_r4127125665
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java:
##########
@@ -618,20 +746,85 @@ synchronized ExpireLockResult
tryExpireCurrentLock(boolean fromShutdownHook) {
}
/**
- * Renews (heartbeats) the current lock if we are the holder, it forcefully
set
- * the expiration flag
- * to false and the lock expiration time to a later time in the future.
+ * Whether storage now holds the expired version of the lock we are
releasing, i.e. whether an
+ * earlier expire attempt that was answered with an error actually landed.
+ */
+ private boolean earlierExpireWriteLanded() {
+ Pair<LockGetResult, Option<StorageLockFile>> current;
+ try {
+ current = storageLockClient.readCurrentLockFile();
+ } catch (RuntimeException e) {
+ // Without a readable lock file we cannot prove the earlier write
landed, so report the failure.
+ logger.warn("Owner {}: Could not read lock {} to check whether an
earlier expire write landed.",
+ ownerId, lockFilePath, e);
+ return false;
Review Comment:
🤖 If this returns false because the read threw, the caller drops into the
ACQUIRED_BY_OTHERS branch. That branch logs "acquired by another owner ...
heartbeat failure" and bumps the acquired-by-others metric. Now that
flaky-network failures come through here, could "could not verify" be reported
separately so a connection blip isn't read as a steal?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/StorageBasedLockProvider.java:
##########
@@ -586,19 +697,36 @@ synchronized ExpireLockResult
tryExpireCurrentLock(boolean fromShutdownHook) {
switch (result.getLeft()) {
case UNKNOWN_ERROR:
// Here we do not know the state of the lock.
- logErrorLockState(FAILED_TO_RELEASE, "Lock state is unknown.");
+ logWarnLockState(FAILED_TO_RELEASE, "Lock state is unknown.");
hoodieLockMetrics.ifPresent(HoodieLockMetrics::updateLockStateUnknownMetric);
- return ExpireLockResult.FAILED;
+ return ExpireLockResult.UNKNOWN_ERROR;
Review Comment:
🤖 Timeouts are probably the most common UNKNOWN_ERROR. The S3 and Azure
clients set a per-call timeout of validity/5 (60s by default) with SDK retries
off, so against a hung endpoint unlock() could now block for about 4 x 60s + 7s
instead of about 60s. Would it make sense to stop retrying once the lease
(`validUntil`) has passed, or to cap the total time?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
