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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
