rangareddy opened a new pull request, #19486:
URL: https://github.com/apache/hudi/pull/19486
### Describe the issue this Pull Request addresses
Part of #16943 (HUDI-9254), "Ensure lock providers are thread safe". That
issue covers five providers and
the close-during-unlock semantics; **this PR fixes one concrete,
self-contained defect in
`FileSystemBasedLockProvider` and deliberately leaves the rest.** What still
needs doing is listed at the
bottom.
`tryLock`, `unlock` and `close` all guarded their lock-file operations with:
```java
private static final String LOCK_FILE_NAME = "lock";
...
synchronized (LOCK_FILE_NAME) { ... }
```
`LOCK_FILE_NAME` is a compile-time String constant, so it is interned. The
monitor is therefore the
JVM-wide canonical `"lock"` instance, shared with every other `"lock"`
literal in the process — Hudi's,
Spark's, or the user's. Any code that happens to synchronize on that literal
blocks Hudi from acquiring or
releasing its lock, and Hudi blocks it in turn.
This is not hypothetical. `FileSystemBasedLockProviderTestClass`, in this
repo, declares:
```java
private static final String LOCK = "lock"; // same interned object as
LOCK_FILE_NAME
```
and synchronizes on it. Two classes with no relationship share one monitor
purely because they chose the
same string.
The correct form already exists in the codebase — `KafkaConnectControlAgent`
uses
`private static final Object LOCK = new Object()`.
### Summary and Changelog
- Guard on a private `LOCK_FILE_MONITOR` object. It is **kept static**, so
the mutual-exclusion scope is
exactly what it was; the only thing that changes is which object is used,
and that it can no longer be
aliased from outside the class.
- `currentOwnerLockInfo` becomes `volatile`. It is written while holding the
monitor but read through the
Lombok `@Getter` without it — `LockManager` reads it to report the current
lock holder — so the value was
not guaranteed visible to the reading thread.
### Verification
New `testAcquisitionIsNotBlockedByTheInternedLockLiteral` starts a thread
that does nothing but
`synchronized ("lock") { … }`, standing in for any unrelated code in the
JVM, then acquires the lock from
another thread. With the fix it acquires immediately. With `synchronized
(LOCK_FILE_NAME)` restored it
blocks for the full budget and the test fails:
```
[ERROR] testAcquisitionIsNotBlockedByTheInternedLockLiteral -- Time elapsed:
10.02 s <<< ERROR!
java.util.concurrent.TimeoutException
```
That 10-second block is the defect, reproduced.
`TestFileSystemBasedLockProvider` 10 tests green, and the whole
`org.apache.hudi.client.transaction.**`
package 256 tests green — worth running in full here because
`FileSystemBasedLockProviderTestClass` aliased
this monitor, so anything that had come to depend on the accidental coupling
would show up there.
`checkstyle:check` and `apache-rat:check` clean.
### Still open under HUDI-9254, not in this PR
- **The monitor is static**, so all `FileSystemBasedLockProvider` instances
in a JVM serialise even for
different tables and different lock files. Narrowing it to per-instance
would be correct only if
`acquireLock` is genuinely atomic for every supported scheme — the
constructor does reject schemes
without atomic creation, so it looks safe, but that is a behaviour change
in a lock provider and wants a
maintainer's call rather than mine.
- **The other providers.** `BaseZookeeperBasedLockProvider` and
`HiveMetastoreBasedLockProvider` have since
had their mutable fields made `volatile`, which fixes visibility but not
the check-then-act sequences
around them (for example ZK's `ValidationUtils.checkArgument(this.lock ==
null, …)` followed by creating
and assigning a new mutex). `DynamoDBBasedLockProvider` still holds its
lock item as instance state.
- **The close-during-unlock semantics** the Jira calls out, which is the
part that actually decides whether
sharing one provider across threads is supported at all.
I would rather land the unambiguous fix than bundle it with changes that
need a design decision. Happy to
follow up on any of the above.
### Impact
No API, config or table format change. For a JVM where nothing else
synchronizes on `"lock"`, behaviour is
identical. Where something does, Hudi's lock operations stop being blocked
by it.
### Risk Level
low — one monitor object swapped for a private one at the same scope, plus a
`volatile`. The scope is
deliberately unchanged, and the regression surface is covered by the full
transaction/lock package.
### 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
- [x] CI passes on my PR
--
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]
