You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "linliu-code (via GitHub)" <gi...@apache.org> on 2026/10/05 23:38:34 UTC
[PR] fix(lock): always close the archiver's lock provider; file-system locks are deleted only by their owner [hudi]
linliu-code opened a new pull request, #20206:
URL: https://github.com/apache/hudi/pull/20206
### Describe the issue this Pull Request addresses
Follow-up to #20163, and stacked on it: the first commit here is #20163's commit, and this PR's own change is the second commit.
#20163 gates the timeline archiver's lock release and close on "this call acquired the lock", so that an archiver which lost the lock race cannot delete the holder's lock. Skipping close() on a failed acquisition, though, leaks the lock provider that attempt created. For StorageBasedLockProvider that is a heartbeat executor and a JVM shutdown hook on every contended archive run.
### Summary and Changelog
- The archivers (V1 and V2) close their transaction manager on every exit path. The release (endStateChange) is still gated on the lock having been acquired.
- Closing without the lock is safe because that call's provider instance holds no lock. The one provider where this did not hold was FileSystemBasedLockProvider: its unlock() and close() deleted the lock file whoever owned it. It now records the content it wrote when it acquired the lock, and deletes the file only while the file still has that content (or a prefix of it, after an interrupted write).
- The record is cleared only once the file is gone or belongs to another owner, so close() still retries a release that failed on an I/O error.
- Tests:
- TestFileSystemBasedLockProvider: a non-holder's unlock and close keep the holder's lock; a former holder does not delete a lock taken over by another owner; an owner still releases a lock file left with partial content.
- TestHoodieTimelineArchiver: after a failed acquisition, the provider is closed and no unlock is attempted.
- The ownership and archiver tests fail without this change.
### Impact
No API or config change. A failed archive lock acquisition no longer leaks its lock provider. FileSystemBasedLockProvider no longer deletes a lock file another writer holds.
### Risk Level
low. It changes only the archiver's cleanup path and FileSystemBasedLockProvider's release. Verified locally: the lock and archiver test classes pass, and checkstyle is clean.
### Documentation Update
none
### Contributor's checklist
- [x] Read through [contributor's guide](https://hudi.apache.org/contribute/how-to-contribute)
- [x] Enough context is provided in the sections above
- [x] Adequate tests were added if applicable
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(lock): always close the archiver's lock provider; file-system locks are deleted only by their owner [hudi]
Posted by "hudi-agent (via GitHub)" <gi...@apache.org>.
hudi-agent commented on code in PR #20206:
URL: https://github.com/apache/hudi/pull/20206#discussion_r4235355161
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -179,15 +183,35 @@ public boolean tryLock(long time, TimeUnit unit) {
public void unlock() {
synchronized (LOCK_FILE_MONITOR) {
try {
- if (storage.exists(this.lockFile)) {
- storage.deleteFile(this.lockFile);
- }
+ deleteOwnLockFile();
} catch (IOException io) {
throw new HoodieIOException(generateLogStatement(LockState.FAILED_TO_RELEASE), io);
}
}
}
+ private void deleteOwnLockFile() throws IOException {
+ String ownContent = ownLockContent;
+ if (ownContent == null) {
+ return;
+ }
+ String currentContent;
+ try (InputStream is = storage.open(this.lockFile)) {
+ currentContent = FileIOUtils.readAsUTFString(is);
+ } catch (FileNotFoundException e) {
+ ownLockContent = null;
+ return;
+ }
+ // A write interrupted after the exclusive create leaves a prefix of the content this instance meant to write.
+ if (ownContent.startsWith(currentContent)) {
Review Comment:
🤖 non-blocking: An empty or half-written file is a prefix of every owner's content, since all of them start `{"lockCreateTime" : "<date>`. On HDFS a new owner's lock file reads as empty from `create` until `close`, so a former holder whose lock expired and was taken over can still delete the new owner's lock if it unlocks in that window. Could the prefix match apply only when this instance's own write failed, with an exact `equals` match once `acquireLock` finished writing?
```java
boolean mine = ownWriteComplete ? ownContent.equals(currentContent) : ownContent.startsWith(currentContent);
```
<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/FileSystemBasedLockProvider.java:
##########
@@ -179,15 +183,35 @@ public boolean tryLock(long time, TimeUnit unit) {
public void unlock() {
synchronized (LOCK_FILE_MONITOR) {
try {
- if (storage.exists(this.lockFile)) {
- storage.deleteFile(this.lockFile);
- }
+ deleteOwnLockFile();
} catch (IOException io) {
throw new HoodieIOException(generateLogStatement(LockState.FAILED_TO_RELEASE), io);
}
}
}
+ private void deleteOwnLockFile() throws IOException {
+ String ownContent = ownLockContent;
+ if (ownContent == null) {
Review Comment:
🤖 Good catch: the old recovery came from the very delete this PR removes, where a contending writer's `LockManager.close()` → `provider.close()` deleted the stale file whoever owned it, so it can't come back without also deleting live holders' locks, and docs are the right fix here; the `LOCK_FILE_MONITOR` javadoc also still mentions "`unlock`'s exists-then-delete and `close`'s unconditional delete" and could be updated too.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(lock): always close the archiver's lock provider; file-system locks are deleted only by their owner [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #20206:
URL: https://github.com/apache/hudi/pull/20206#issuecomment-6005860983
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "5637446c4e92d203353e4db76decb51272b8977a",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "5637446c4e92d203353e4db76decb51272b8977a",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 5637446c4e92d203353e4db76decb51272b8977a UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(lock): always close the archiver's lock provider; file-system locks are deleted only by their owner [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #20206:
URL: https://github.com/apache/hudi/pull/20206#issuecomment-6008407169
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "5637446c4e92d203353e4db76decb51272b8977a",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=17472",
"triggerID" : "5637446c4e92d203353e4db76decb51272b8977a",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 5637446c4e92d203353e4db76decb51272b8977a Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=17472)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(lock): always close the archiver's lock provider; file-system locks are deleted only by their owner [hudi]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #20206:
URL: https://github.com/apache/hudi/pull/20206#discussion_r4202966684
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -179,15 +183,35 @@ public boolean tryLock(long time, TimeUnit unit) {
public void unlock() {
synchronized (LOCK_FILE_MONITOR) {
try {
- if (storage.exists(this.lockFile)) {
- storage.deleteFile(this.lockFile);
- }
+ deleteOwnLockFile();
} catch (IOException io) {
throw new HoodieIOException(generateLogStatement(LockState.FAILED_TO_RELEASE), io);
}
}
}
+ private void deleteOwnLockFile() throws IOException {
+ String ownContent = ownLockContent;
+ if (ownContent == null) {
Review Comment:
With `hoodie.write.lock.filesystem.expire` at its default of 0, nothing removes a lock file left by a crashed writer any more, so every later write on the table fails until someone deletes the file by hand - is that intended? If so, the class javadoc's "if writeClient crash and never run again" and the expire config's doc should say that crash recovery now needs an expiry or manual cleanup.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(lock): always close the archiver's lock provider; file-system locks are deleted only by their owner [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #20206:
URL: https://github.com/apache/hudi/pull/20206#issuecomment-6005980165
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "5637446c4e92d203353e4db76decb51272b8977a",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=17472",
"triggerID" : "5637446c4e92d203353e4db76decb51272b8977a",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 5637446c4e92d203353e4db76decb51272b8977a Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=17472)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org