hudi-agent commented on code in PR #20163:
URL: https://github.com/apache/hudi/pull/20163#discussion_r4147261526
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/timeline/TimelineArchiverV1.java:
##########
@@ -169,9 +171,17 @@ public int archiveIfRequired(HoodieEngineContext context,
boolean acquireLock) t
metrics.put(failureMetricName, 1L);
throw e;
} finally {
- close();
- if (acquireLock) {
- txnManager.endStateChange(Option.empty());
+ try {
+ close();
+ if (lockAcquired) {
+ txnManager.endStateChange(Option.empty());
+ }
+ } finally {
+ // A no-op after endStateChange; releases the provider when a failed
writer close skipped it.
+ // Some providers delete the lock on close, so never close a lock this
call did not acquire.
+ if (lockAcquired) {
Review Comment:
🤖 When `beginStateChange` fails (lock timeout under contention), this path
now also skips `txnManager.close()`. Before, the finally went `endStateChange`
→ `LockManager.unlock()` → `close()`. So for V1 this looks like a new leak of
the archiver's private provider on every contended archival: the ZK curator
client, the HMS heartbeat executor, or `StorageBasedLockProvider`'s JVM
shutdown hook. Could `FileSystemBasedLockProvider.close()` delete only a lock
it acquired, so that this path can always close?
<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/LockManager.java:
##########
@@ -95,14 +95,19 @@ public void lock() {
* and tries to call unlock()
*/
public void unlock() {
- getLockProvider().unlock();
+ // Close even when the release fails, so the next lock() builds a fresh
provider instead of
+ // reusing one whose lock state is no longer trustworthy.
try {
- metrics.updateLockHeldTimerMetrics();
- } catch (HoodieException e) {
- log.error(String.format("Exception encountered when updating lock
metrics: %s", e));
+ getLockProvider().unlock();
+ try {
+ metrics.updateLockHeldTimerMetrics();
+ } catch (HoodieException e) {
+ log.error(String.format("Exception encountered when updating lock
metrics: %s", e));
+ }
+ metrics.updateLockReleaseSuccessMetric();
+ } finally {
+ close();
Review Comment:
🤖 The javadoc above says unlock() may be called by a thread that doesn't
hold the lock. For ZK, `InterProcessMutex.release()` from a non-owner thread
throws, and this `close()` then closes the shared curator client, which drops
the real holder's ephemeral lock node. Before this change that failure left the
holder's lock alone. Can this happen when two threads share a
TransactionManager, e.g. HoodieStreamer async compaction failing to begin while
the scheduling thread holds the lock with an empty owner? @nsivabalan could you
sanity-check this?
<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]
