You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "rangareddy (via GitHub)" <gi...@apache.org> on 2026/08/03 10:47:26 UTC
[PR] fix(lock): stop guarding the filesystem lock with an interned string literal [hudi]
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(lock): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "hudi-agent (via GitHub)" <gi...@apache.org>.
hudi-agent commented on code in PR #19486:
URL: https://github.com/apache/hudi/pull/19486#discussion_r3703317991
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -63,14 +63,30 @@
@Slf4j
public class FileSystemBasedLockProvider implements LockProvider<String>, Serializable {
private static final String LOCK_FILE_NAME = "lock";
+ /**
+ * Guards this provider's lock-file operations.
+ *
+ * <p>These blocks used to synchronize on {@link #LOCK_FILE_NAME}. That is a compile-time String
+ * constant, so it is interned and shared JVM-wide with every other {@code "lock"} literal - including
+ * {@code FileSystemBasedLockProviderTestClass}, which declares its own {@code static final String LOCK =
Review Comment:
🤖 nit: referencing a test class by name in production Javadoc is a bit fragile — if `FileSystemBasedLockProviderTestClass` is ever renamed, this comment silently goes stale. Could you drop the specific class name and just say something like "any class in the same JVM that interns the same `\"lock\"` literal contends on the very same monitor"?
<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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(lock): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5253234774
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989",
"triggerID" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"triggerType" : "PUSH"
}, {
"hash" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16029",
"triggerID" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"triggerType" : "PUSH"
}, {
"hash" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16221",
"triggerID" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* a639f7364212bab80b7ccf398a2808976732a7d5 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16221)
<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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5175354268
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989",
"triggerID" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"triggerType" : "PUSH"
}, {
"hash" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 1556e0d14f3d913b9c9c5381e3b69473e3741902 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989)
* 9d0b7756bff92dd510601481a1397fab6a3a57a1 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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5252286921
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989",
"triggerID" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"triggerType" : "PUSH"
}, {
"hash" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16029",
"triggerID" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"triggerType" : "PUSH"
}, {
"hash" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16221",
"triggerID" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 9d0b7756bff92dd510601481a1397fab6a3a57a1 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16029)
* a639f7364212bab80b7ccf398a2808976732a7d5 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16221)
<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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5177328657
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989",
"triggerID" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"triggerType" : "PUSH"
}, {
"hash" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16029",
"triggerID" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 9d0b7756bff92dd510601481a1397fab6a3a57a1 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16029)
<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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5372169119
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989",
"triggerID" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"triggerType" : "PUSH"
}, {
"hash" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16029",
"triggerID" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"triggerType" : "PUSH"
}, {
"hash" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16221",
"triggerID" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"triggerType" : "PUSH"
}, {
"hash" : "58219469d783bb39fa562117550fe2682a8e7486",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16395",
"triggerID" : "58219469d783bb39fa562117550fe2682a8e7486",
"triggerType" : "PUSH"
}, {
"hash" : "771e03fa09a94f284889d9089ccbbd7cd5f946f7",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16465",
"triggerID" : "771e03fa09a94f284889d9089ccbbd7cd5f946f7",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 58219469d783bb39fa562117550fe2682a8e7486 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16395)
* 771e03fa09a94f284889d9089ccbbd7cd5f946f7 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16465)
<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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5171998937
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989",
"triggerID" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 1556e0d14f3d913b9c9c5381e3b69473e3741902 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989)
<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): stop guarding the filesystem lock with an interned string literal and make the provider serializable [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous merged PR #19486:
URL: https://github.com/apache/hudi/pull/19486
--
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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5372293661
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989",
"triggerID" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"triggerType" : "PUSH"
}, {
"hash" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16029",
"triggerID" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"triggerType" : "PUSH"
}, {
"hash" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16221",
"triggerID" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"triggerType" : "PUSH"
}, {
"hash" : "58219469d783bb39fa562117550fe2682a8e7486",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16395",
"triggerID" : "58219469d783bb39fa562117550fe2682a8e7486",
"triggerType" : "PUSH"
}, {
"hash" : "771e03fa09a94f284889d9089ccbbd7cd5f946f7",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16465",
"triggerID" : "771e03fa09a94f284889d9089ccbbd7cd5f946f7",
"triggerType" : "PUSH"
}, {
"hash" : "428a7197aee0cd5114ef21112448d838c80ad9da",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "428a7197aee0cd5114ef21112448d838c80ad9da",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 58219469d783bb39fa562117550fe2682a8e7486 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16395)
* 771e03fa09a94f284889d9089ccbbd7cd5f946f7 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16465)
* 428a7197aee0cd5114ef21112448d838c80ad9da 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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5166769247
## [Codecov](https://app.codecov.io/gh/apache/hudi/pull/19486?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 76.96%. Comparing base ([`637996c`](https://app.codecov.io/gh/apache/hudi/commit/637996c5ab2078dfd858a2a425da8f28af1ce789?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`1556e0d`](https://app.codecov.io/gh/apache/hudi/commit/1556e0d14f3d913b9c9c5381e3b69473e3741902?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19486 +/- ##
============================================
- Coverage 76.96% 76.96% -0.01%
+ Complexity 33851 33848 -3
============================================
Files 2575 2575
Lines 143378 143379 +1
Branches 17573 17573
============================================
- Hits 110349 110348 -1
- Misses 24765 24766 +1
- Partials 8264 8265 +1
```
| [Components](https://app.codecov.io/gh/apache/hudi/pull/19486/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hudi-common](https://app.codecov.io/gh/apache/hudi/pull/19486/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `82.25% <ø> (-0.01%)` | :arrow_down: |
| [hudi-client](https://app.codecov.io/gh/apache/hudi/pull/19486/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.82% <100.00%> (+<0.01%)` | :arrow_up: |
| [hudi-flink](https://app.codecov.io/gh/apache/hudi/pull/19486/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.96% <ø> (ø)` | |
| [hudi-spark-datasource](https://app.codecov.io/gh/apache/hudi/pull/19486/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `75.11% <ø> (ø)` | |
| [hudi-utilities](https://app.codecov.io/gh/apache/hudi/pull/19486/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.63% <ø> (ø)` | |
| [hudi-cli](https://app.codecov.io/gh/apache/hudi/pull/19486/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `15.32% <ø> (ø)` | |
| [hudi-hadoop](https://app.codecov.io/gh/apache/hudi/pull/19486/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.49% <ø> (ø)` | |
| [hudi-sync](https://app.codecov.io/gh/apache/hudi/pull/19486/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.87% <ø> (ø)` | |
| [hudi-io](https://app.codecov.io/gh/apache/hudi/pull/19486/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `79.55% <ø> (ø)` | |
| [hudi-timeline-service](https://app.codecov.io/gh/apache/hudi/pull/19486/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.74% <ø> (ø)` | |
| [hudi-cloud](https://app.codecov.io/gh/apache/hudi/pull/19486/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `64.00% <ø> (ø)` | |
| [hudi-kafka-connect](https://app.codecov.io/gh/apache/hudi/pull/19486/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.20% <ø> (ø)` | |
| [Flag](https://app.codecov.io/gh/apache/hudi/pull/19486/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hadoop-mr-java-client](https://app.codecov.io/gh/apache/hudi/pull/19486/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `43.71% <100.00%> (-0.07%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/19486?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../transaction/lock/FileSystemBasedLockProvider.java](https://app.codecov.io/gh/apache/hudi/pull/19486?src=pr&el=tree&filepath=hudi-client%2Fhudi-client-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fclient%2Ftransaction%2Flock%2FFileSystemBasedLockProvider.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jbGllbnQvaHVkaS1jbGllbnQtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWRpL2NsaWVudC90cmFuc2FjdGlvbi9sb2NrL0ZpbGVTeXN0ZW1CYXNlZExvY2tQcm92aWRlci5qYXZh) | `77.17% <100.00%> (+0.25%)` | :arrow_up: |
... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/hudi/pull/19486/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</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): stop guarding the filesystem lock with an interned string literal and make the provider serializable [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5372988122
Notes for reviewers and the merging committer, moved out of the description to keep it readable.
**Mutation ledger.** Every production change is pinned by a test that was run against the reverted change to confirm it fails: the three monitor blocks (each reverted alone times out the interned-literal test), `create(path, true)` fails the atomic-create test, the pre-fix reload fails the vanished-lock-file test, a widened `catch (IOException)` fails the IO-failure test, a throw-and-clear catch fails its keep-the-owner assertion, non-transient `lockInfo` fails the round trip with `NotSerializableException`, the removed `""` initializer fails the failing-create test, and both `checkIfExpired` mutants (treat-as-expired, rethrow) fail the stat-failure test.
**Serialization compatibility.** 0.12.x is the only release line that ever serialized this class (HUDI-5377 landed in 0.13.0). Its streams carry a computed `serialVersionUID` (`1687524493539720009`) that no 0.13.0+ build could read anyway (0.13.0 computes `-7877409577963519069`), and no Hudi code persists a provider outside intra-job closure capture. Pinning `1L` therefore cannot break a readable stream.
**Squash message.** This branch's four commit messages tell a self-correcting story (the third commit's message contains two claims its successor corrects), and the default concatenated squash body would carry all of them into master verbatim. Please replace the squash message body with:
```
FileSystemBasedLockProvider synchronized tryLock/unlock/close on the "lock" String
literal. Compile-time constants are interned, so any class in the JVM synchronizing
on the same literal contends on the very same monitor; the in-repo test helper
FileSystemBasedLockProviderTestClass did exactly that, waiting on the monitor the
production provider takes. The guard is a private static Object now, same scope
within a classloader, no aliasing. The test helper got the same fix.
Also fixed while in the file, each pinned by a test that fails without it:
- reloadCurrentOwnerLockInfo evaluated storage.open in its try-with-resources
header, so a vanished lock file threw out of the reload instead of clearing the
field, and LockManager reported the previous owner as current. The reload now
treats FileNotFoundException (at open, or at first read on lazily fetching
stores) as "no owner" and lets any other IOException escape as HoodieIOException.
- The class declared Serializable but every instance since 0.13.0 (HUDI-5377)
failed to serialize: lockInfo held a non-Serializable LockInfo built eagerly in
the constructor, breaking Spark closure capture (the HUDI-7782 bug class).
lockInfo and sdf are transient and lazily rebuilt under the monitor;
serialVersionUID is pinned at 1L.
- currentOwnerLockInfo is volatile (getCurrentOwnerLockInfo is public LockProvider
API and may be read from another thread) and defaults to "" so LockManager never
logs a null owner.
- acquireLock is package-private @VisibleForTesting: storage.create(path, false)
is the provider's cross-process mutual exclusion on stores with atomic create,
and no public-API test can reach its already-exists arm.
- checkIfExpired documents that a stat failure degrades to "not expired", the same
fail-safe choice as StorageBasedLockProvider's UNKNOWN_ERROR arm, and its catch
logs ACQUIRING instead of the contradictory ALREADY_RELEASED.
- HoodieStorage.open documents the FileNotFoundException contract the reload
relies on; TestHoodieStorageBase already asserts it.
New tests: interned-monitor non-blocking for tryLock/unlock/close, atomic-create
loser with the winner's payload intact, tryLock returning false when the create
fails, reload clearing on a vanished lock file, reload keeping the last known
owner on a real IO error, a stat failure during the expiry check neither stealing
a live lock nor skipping the owner reload, and a Java serialization round trip of
a lock-holding provider.
Part of #16943
```
--
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): stop guarding the filesystem lock with an interned string literal and make the provider serializable [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5372965812
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989",
"triggerID" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"triggerType" : "PUSH"
}, {
"hash" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16029",
"triggerID" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"triggerType" : "PUSH"
}, {
"hash" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16221",
"triggerID" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"triggerType" : "PUSH"
}, {
"hash" : "58219469d783bb39fa562117550fe2682a8e7486",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16395",
"triggerID" : "58219469d783bb39fa562117550fe2682a8e7486",
"triggerType" : "PUSH"
}, {
"hash" : "771e03fa09a94f284889d9089ccbbd7cd5f946f7",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16465",
"triggerID" : "771e03fa09a94f284889d9089ccbbd7cd5f946f7",
"triggerType" : "PUSH"
}, {
"hash" : "428a7197aee0cd5114ef21112448d838c80ad9da",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16467",
"triggerID" : "428a7197aee0cd5114ef21112448d838c80ad9da",
"triggerType" : "PUSH"
}, {
"hash" : "831aed976cbfaa8dcb64f8f9c490aa95d86494f9",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "831aed976cbfaa8dcb64f8f9c490aa95d86494f9",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 771e03fa09a94f284889d9089ccbbd7cd5f946f7 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16465)
* 428a7197aee0cd5114ef21112448d838c80ad9da Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16467)
* 831aed976cbfaa8dcb64f8f9c490aa95d86494f9 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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5349377109
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989",
"triggerID" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"triggerType" : "PUSH"
}, {
"hash" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16029",
"triggerID" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"triggerType" : "PUSH"
}, {
"hash" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16221",
"triggerID" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"triggerType" : "PUSH"
}, {
"hash" : "58219469d783bb39fa562117550fe2682a8e7486",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "58219469d783bb39fa562117550fe2682a8e7486",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* a639f7364212bab80b7ccf398a2808976732a7d5 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16221)
* 58219469d783bb39fa562117550fe2682a8e7486 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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5371988235
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989",
"triggerID" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"triggerType" : "PUSH"
}, {
"hash" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16029",
"triggerID" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"triggerType" : "PUSH"
}, {
"hash" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16221",
"triggerID" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"triggerType" : "PUSH"
}, {
"hash" : "58219469d783bb39fa562117550fe2682a8e7486",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16395",
"triggerID" : "58219469d783bb39fa562117550fe2682a8e7486",
"triggerType" : "PUSH"
}, {
"hash" : "771e03fa09a94f284889d9089ccbbd7cd5f946f7",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "771e03fa09a94f284889d9089ccbbd7cd5f946f7",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 58219469d783bb39fa562117550fe2682a8e7486 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16395)
* 771e03fa09a94f284889d9089ccbbd7cd5f946f7 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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19486:
URL: https://github.com/apache/hudi/pull/19486#discussion_r3773085020
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestFileSystemBasedLockProvider.java:
##########
@@ -81,6 +86,60 @@ public void testAcquireAndReleaseLock() {
}
}
+ /**
+ * The lock-file operations used to synchronize on the {@code "lock"} String constant, which is interned
+ * and therefore shared JVM-wide with every other {@code "lock"} literal. Unrelated code holding that
+ * monitor blocked lock acquisition outright. {@code FileSystemBasedLockProviderTestClass} in this very
+ * repo declares {@code static final String LOCK = "lock"} and so aliases it.
+ */
+ @Test
+ public void testAcquisitionIsNotBlockedByTheInternedLockLiteral() throws Exception {
+ StorageConfiguration<?> storageConf = HoodieTestUtils.getDefaultStorageConf();
+ FileSystemBasedLockProvider provider =
+ new FileSystemBasedLockProvider(lockConfiguration(lockDir("interned"), 0), storageConf);
+ CountDownLatch holding = new CountDownLatch(1);
+ CountDownLatch release = new CountDownLatch(1);
+ // stands in for any other class in the JVM doing synchronized ("lock")
+ Thread unrelated = new Thread(() -> {
+ synchronized ("lock") {
+ holding.countDown();
+ try {
+ release.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ });
+ unrelated.setDaemon(true);
+ unrelated.start();
+ assertTrue(holding.await(10, TimeUnit.SECONDS), "the unrelated thread should hold the interned monitor");
+
+ try {
+ ExecutorService executor = Executors.newSingleThreadExecutor();
+ try {
+ Future<Boolean> acquired = executor.submit(() -> provider.tryLock(1, TimeUnit.SECONDS));
+ boolean gotLock;
+ try {
+ gotLock = acquired.get(10, TimeUnit.SECONDS);
+ } catch (TimeoutException e) {
+ // The bare TimeoutException says nothing about why, and this is the failure the regression
+ // produces: tryLock is parked on a monitor an unrelated thread holds, so it never returns.
+ throw new AssertionError("tryLock never returned - it is blocked on the monitor held by the "
+ + "unrelated thread, which means the provider is synchronizing on the interned \"lock\" "
+ + "literal again rather than on a private monitor", e);
+ }
+ assertTrue(gotLock,
+ "acquisition must not wait on a monitor held by code that has nothing to do with Hudi");
+ } finally {
+ executor.shutdownNow();
+ }
+ } finally {
+ release.countDown();
+ provider.unlock();
+ provider.close();
Review Comment:
The test guards `tryLock` only, not the other two blocks this PR converts.
`release.countDown()` runs before `provider.unlock()` and `provider.close()`, so neither ever executes while the unrelated thread holds the interned monitor. Restore `synchronized (LOCK_FILE_NAME)` in only `unlock()` (L160) or only `close()` (L115) and this test still passes. Three blocks converted, one covered.
Put the release path through the same gate before dropping the latch:
```suggestion
assertTrue(gotLock,
"acquisition must not wait on a monitor held by code that has nothing to do with Hudi");
// unlock() and close() take the same monitor, so exercise them while it is still held.
Future<?> released = executor.submit(() -> {
provider.unlock();
provider.close();
});
try {
released.get(10, TimeUnit.SECONDS);
} catch (TimeoutException e) {
throw new AssertionError("unlock/close never returned - they are blocked on the monitor held "
+ "by the unrelated thread, which means unlock()/close() are synchronizing on the interned "
+ "\"lock\" literal again rather than on a private monitor", e);
}
} finally {
executor.shutdownNow();
}
} finally {
release.countDown();
provider.unlock();
provider.close();
```
The repeated `unlock()`/`close()` in the outer finally stays safe: `testUnlockWithoutLockIsNoOp` already covers both being no-ops when no lock file is present.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -63,14 +63,29 @@
@Slf4j
public class FileSystemBasedLockProvider implements LockProvider<String>, Serializable {
private static final String LOCK_FILE_NAME = "lock";
+ /**
+ * Guards this provider's lock-file operations.
+ *
+ * <p>These blocks used to synchronize on {@link #LOCK_FILE_NAME}. That is a compile-time String constant,
+ * so it is interned: any class anywhere in the JVM that synchronizes on the same {@code "lock"} literal
+ * contends on the very same monitor and silently couples itself to Hudi's lock acquisition. A private
+ * object cannot be aliased that way.
+ *
+ * <p>Kept static so the mutual-exclusion scope is unchanged by this fix.
Review Comment:
"the mutual-exclusion scope is unchanged" is not quite true, and the exception is reachable.
An interned literal lives in the JVM-wide string table and is shared across every classloader (JLS 3.10.5); a `static final Object` is per loaded class. Where two copies of this class are loaded, the old monitor was shared and the new one is not.
That configuration exists in practice: `StreamerUtil.java:235-241` installs this provider as Flink's default when a lock is required and none is configured, and a Flink session cluster gives each job its own user-code classloader inside one TaskManager JVM.
This is not a correctness regression for supported deployments, since the atomic create on storage is the real mutual exclusion and `acquireLock()` handles the loser through `FileAlreadyExistsException` -> `HoodieIOException` -> `return false` at L152. But this comment is what the next reader will trust, so it should state what is actually guaranteed:
```suggestion
* <p>Kept static so that, within a classloader, the mutual-exclusion scope is unchanged by this fix.
* The interned literal was additionally shared across classloaders; that was never a correctness
* guarantee, since the atomic create on storage is the real mutual exclusion.
```
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -63,14 +63,29 @@
@Slf4j
public class FileSystemBasedLockProvider implements LockProvider<String>, Serializable {
private static final String LOCK_FILE_NAME = "lock";
+ /**
+ * Guards this provider's lock-file operations.
+ *
+ * <p>These blocks used to synchronize on {@link #LOCK_FILE_NAME}. That is a compile-time String constant,
+ * so it is interned: any class anywhere in the JVM that synchronizes on the same {@code "lock"} literal
+ * contends on the very same monitor and silently couples itself to Hudi's lock acquisition. A private
+ * object cannot be aliased that way.
+ *
+ * <p>Kept static so the mutual-exclusion scope is unchanged by this fix.
+ */
+ private static final Object LOCK_FILE_MONITOR = new Object();
private final int lockTimeoutMinutes;
private final transient HoodieStorage storage;
private final transient StoragePath lockFile;
protected LockConfiguration lockConfiguration;
private final SimpleDateFormat sdf;
private final LockInfo lockInfo;
+ /**
+ * Written while holding {@link #LOCK_FILE_MONITOR} in {@code tryLock}, but read through the generated
+ * getter without it, so the read needs to be volatile for the value to be visible to other threads.
+ */
@Getter
- private String currentOwnerLockInfo;
+ private volatile String currentOwnerLockInfo;
Review Comment:
Three notes on the "Still open under HUDI-9254" section, since that list is what the next person will work from.
1. "`BaseZookeeperBasedLockProvider` and `HiveMetastoreBasedLockProvider` **have since** had their mutable fields made `volatile`" -- they were volatile in their original commits, not added later. `git show 74241947c123 -- '*ZookeeperBasedLockProvider.java'` (HUDI-845, #2374) introduces `private volatile InterProcessMutex lock = null;`, and `git show d7b18783bdd6 -- '*HiveMetastoreBasedLockProvider.java'` introduces `private volatile LockResponse lock = null;`. The check-then-act point is correct; only the history is wrong.
2. "`DynamoDBBasedLockProvider` still holds its lock item as instance state" -- true, but that field is volatile too (`DynamoDBBasedLockProviderBase.java:80`, `protected volatile LockItem lock`), so the contrast drawn with ZK and Hive does not hold. Its defect is the same check-then-act shape, not visibility.
3. Worth adding to punted item (a): the reason the monitor has to stay static is the expiry path, not the create path. Two threads can both evaluate `checkIfExpired()` true at L141, both call `deleteFile` at L142, and the second delete removes the lock file the first just created, leaving both to return true from `storage.exists` at L150. Stating that converts the hedge into an invariant and tells whoever narrows the scope later exactly what they have to prove.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestFileSystemBasedLockProvider.java:
##########
@@ -81,6 +86,60 @@ public void testAcquireAndReleaseLock() {
}
}
+ /**
+ * The lock-file operations used to synchronize on the {@code "lock"} String constant, which is interned
+ * and therefore shared JVM-wide with every other {@code "lock"} literal. Unrelated code holding that
+ * monitor blocked lock acquisition outright. {@code FileSystemBasedLockProviderTestClass} in this very
+ * repo declares {@code static final String LOCK = "lock"} and so aliases it.
+ */
+ @Test
+ public void testAcquisitionIsNotBlockedByTheInternedLockLiteral() throws Exception {
+ StorageConfiguration<?> storageConf = HoodieTestUtils.getDefaultStorageConf();
+ FileSystemBasedLockProvider provider =
+ new FileSystemBasedLockProvider(lockConfiguration(lockDir("interned"), 0), storageConf);
+ CountDownLatch holding = new CountDownLatch(1);
+ CountDownLatch release = new CountDownLatch(1);
+ // stands in for any other class in the JVM doing synchronized ("lock")
+ Thread unrelated = new Thread(() -> {
+ synchronized ("lock") {
+ holding.countDown();
+ try {
+ release.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ });
+ unrelated.setDaemon(true);
+ unrelated.start();
+ assertTrue(holding.await(10, TimeUnit.SECONDS), "the unrelated thread should hold the interned monitor");
+
+ try {
Review Comment:
If the assertion on L115 fails, the interned monitor is never released.
`unrelated.start()` and the `holding.await` assertion sit outside the `try` whose `finally` calls `release.countDown()`. On assertion failure the daemon thread still enters `synchronized ("lock")` and parks on `release.await()` forever, holding the JVM-wide monitor for the remaining life of the fork -- surefire runs `forkCount=1` / `reuseForks=true` (pom.xml:2212-2213). That turns one red test into a hung module.
Move the start and the await inside the try so the latch always drops:
```suggestion
unrelated.setDaemon(true);
try {
unrelated.start();
assertTrue(holding.await(10, TimeUnit.SECONDS), "the unrelated thread should hold the interned monitor");
```
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -63,14 +63,29 @@
@Slf4j
public class FileSystemBasedLockProvider implements LockProvider<String>, Serializable {
private static final String LOCK_FILE_NAME = "lock";
+ /**
+ * Guards this provider's lock-file operations.
+ *
+ * <p>These blocks used to synchronize on {@link #LOCK_FILE_NAME}. That is a compile-time String constant,
+ * so it is interned: any class anywhere in the JVM that synchronizes on the same {@code "lock"} literal
+ * contends on the very same monitor and silently couples itself to Hudi's lock acquisition. A private
+ * object cannot be aliased that way.
+ *
+ * <p>Kept static so the mutual-exclusion scope is unchanged by this fix.
+ */
+ private static final Object LOCK_FILE_MONITOR = new Object();
private final int lockTimeoutMinutes;
private final transient HoodieStorage storage;
private final transient StoragePath lockFile;
protected LockConfiguration lockConfiguration;
private final SimpleDateFormat sdf;
private final LockInfo lockInfo;
+ /**
+ * Written while holding {@link #LOCK_FILE_MONITOR} in {@code tryLock}, but read through the generated
+ * getter without it, so the read needs to be volatile for the value to be visible to other threads.
+ */
Review Comment:
Two small things about this `volatile`.
The stated reason does not hold in-repo: both readers run on the same thread that called `tryLock`. `LockManager.lock()` (L76-90) goes through `RetryHelper.start`, which invokes `func.get()` inline (`RetryHelper.java:92`, no executor), and `TimeGeneratorBase` has the same shape. `git grep getCurrentOwnerLockInfo` returns only those two call sites plus the interface default and one test. The defensible justification is that `getCurrentOwnerLockInfo()` is public `LockProvider` API and a plugged-in caller may read it from another thread, which is a fine reason to keep the modifier, just not the one written here.
Second, `volatile` on a private instance field changes the class's default `serialVersionUID`, and this class is `Serializable` (L64) with no explicit UID. Compiling the same class name with and without the modifier:
```
baseline 4315981554152328805
+ private static final Object field 4315981554152328805 (private static is excluded from the hash)
+ volatile on the instance field 7607095523244839677
```
That only bites on mixed-version deserialization, and the provider's `storage`/`lockFile` are transient anyway, so I would not change the code. Please reword the Javadoc to the public-API reason, and add a line to the PR description noting the UID change so it is not a surprise later.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestFileSystemBasedLockProvider.java:
##########
@@ -81,6 +86,60 @@ public void testAcquireAndReleaseLock() {
}
}
+ /**
+ * The lock-file operations used to synchronize on the {@code "lock"} String constant, which is interned
+ * and therefore shared JVM-wide with every other {@code "lock"} literal. Unrelated code holding that
+ * monitor blocked lock acquisition outright. {@code FileSystemBasedLockProviderTestClass} in this very
+ * repo declares {@code static final String LOCK = "lock"} and so aliases it.
+ */
+ @Test
+ public void testAcquisitionIsNotBlockedByTheInternedLockLiteral() throws Exception {
+ StorageConfiguration<?> storageConf = HoodieTestUtils.getDefaultStorageConf();
+ FileSystemBasedLockProvider provider =
+ new FileSystemBasedLockProvider(lockConfiguration(lockDir("interned"), 0), storageConf);
+ CountDownLatch holding = new CountDownLatch(1);
+ CountDownLatch release = new CountDownLatch(1);
+ // stands in for any other class in the JVM doing synchronized ("lock")
+ Thread unrelated = new Thread(() -> {
+ synchronized ("lock") {
Review Comment:
nit, feel free to ignore: after this PR the test helper is the only `"lock"` monitor left in the tree.
`FileSystemBasedLockProviderTestClass.java:45` declares `private static final String LOCK = "lock"`, synchronizes on it at :64/:77/:95 and calls `LOCK.wait(retryWaitTimeMs)` at :79. It is no longer a hazard to production now that this provider has its own monitor, but it is the same defect, and this test deliberately adds a second aliaser.
Splitting that field into `private static final Object LOCK = new Object()` for the monitor and a separate `LOCK_FILE_NAME` string for the path removes the bug class from the repo entirely. Worth folding into this PR since it is the same fix; a follow-up is fine too.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestFileSystemBasedLockProvider.java:
##########
@@ -81,6 +86,60 @@ public void testAcquireAndReleaseLock() {
}
}
+ /**
+ * The lock-file operations used to synchronize on the {@code "lock"} String constant, which is interned
+ * and therefore shared JVM-wide with every other {@code "lock"} literal. Unrelated code holding that
+ * monitor blocked lock acquisition outright. {@code FileSystemBasedLockProviderTestClass} in this very
+ * repo declares {@code static final String LOCK = "lock"} and so aliases it.
Review Comment:
nit, feel free to ignore: this is the reference you agreed to drop from the production Javadoc.
The `FileSystemBasedLockProviderTestClass` name and its `static final String LOCK = "lock"` detail survive here after being removed from `FileSystemBasedLockProvider`. Same rot risk, and because it is `{@code}` rather than `{@link}`, a rename of that class fails silently instead of breaking the build.
Either drop the last sentence and keep the mechanism description, or write it as `{@link org.apache.hudi.client.transaction.FileSystemBasedLockProviderTestClass}` so a rename is caught at compile time.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -120,7 +135,7 @@ public void close() {
@Override
public boolean tryLock(long time, TimeUnit unit) {
try {
- synchronized (LOCK_FILE_NAME) {
+ synchronized (LOCK_FILE_MONITOR) {
Review Comment:
Adjacent pre-existing bug in the method this `volatile` publishes.
`reloadCurrentOwnerLockInfo()` (L206-216) evaluates `storage.open(this.lockFile)` in the try-with-resources header, before the `storage.exists` check on L208. `HoodieHadoopStorage.open` is `fs.open(path)`, which throws `FileNotFoundException` on a missing path, so the `else { this.currentOwnerLockInfo = ""; }` on L211 is dead code: a vanished lock file throws instead of resetting the field. The exception is swallowed by this method's caller at L152, the field keeps the previous owner's payload, and `LockManager.java:83` then prints it as "Current lock owner information". Making the field volatile publishes that stale value more reliably rather than fixing it.
This is the surviving half of a two-part regression: `5faefcd01fa8` ([MINOR] #10411) hoisted both `fs.create` and `fs.open` above their guards, and #19222 fixed only the `acquireLock` half.
Either fix it here while you are in the file:
```java
public void reloadCurrentOwnerLockInfo() {
try {
if (!storage.exists(this.lockFile)) {
this.currentOwnerLockInfo = "";
return;
}
try (InputStream is = storage.open(this.lockFile)) {
this.currentOwnerLockInfo = FileIOUtils.readAsUTFString(is);
}
} catch (IOException e) {
throw new HoodieIOException(generateLogStatement(LockState.FAILED_TO_ACQUIRE), e);
}
}
```
or open a separate issue for it and link that from the "Still open" list.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -63,14 +63,29 @@
@Slf4j
public class FileSystemBasedLockProvider implements LockProvider<String>, Serializable {
private static final String LOCK_FILE_NAME = "lock";
+ /**
+ * Guards this provider's lock-file operations.
+ *
+ * <p>These blocks used to synchronize on {@link #LOCK_FILE_NAME}. That is a compile-time String constant,
+ * so it is interned: any class anywhere in the JVM that synchronizes on the same {@code "lock"} literal
+ * contends on the very same monitor and silently couples itself to Hudi's lock acquisition. A private
+ * object cannot be aliased that way.
+ *
+ * <p>Kept static so the mutual-exclusion scope is unchanged by this fix.
+ */
+ private static final Object LOCK_FILE_MONITOR = new Object();
Review Comment:
The commit message trailer will auto-close #16943 on merge.
`git log -1 --format=%B` ends with `Closes #16943`, but the PR description says "Part of #16943 ... this PR fixes one concrete, self-contained defect ... and deliberately leaves the rest". #16943 is open, labelled `priority:critical`, and covers five providers plus the close-during-unlock semantics.
Please reword the trailer to `Part of #16943`. GitHub only auto-closes on `Closes`/`Fixes`/`Resolves`, so that keeps the issue open for the remaining work.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestFileSystemBasedLockProvider.java:
##########
@@ -81,6 +86,60 @@ public void testAcquireAndReleaseLock() {
}
}
+ /**
+ * The lock-file operations used to synchronize on the {@code "lock"} String constant, which is interned
+ * and therefore shared JVM-wide with every other {@code "lock"} literal. Unrelated code holding that
+ * monitor blocked lock acquisition outright. {@code FileSystemBasedLockProviderTestClass} in this very
+ * repo declares {@code static final String LOCK = "lock"} and so aliases it.
+ */
+ @Test
+ public void testAcquisitionIsNotBlockedByTheInternedLockLiteral() throws Exception {
+ StorageConfiguration<?> storageConf = HoodieTestUtils.getDefaultStorageConf();
+ FileSystemBasedLockProvider provider =
+ new FileSystemBasedLockProvider(lockConfiguration(lockDir("interned"), 0), storageConf);
+ CountDownLatch holding = new CountDownLatch(1);
+ CountDownLatch release = new CountDownLatch(1);
+ // stands in for any other class in the JVM doing synchronized ("lock")
+ Thread unrelated = new Thread(() -> {
+ synchronized ("lock") {
+ holding.countDown();
+ try {
+ release.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ });
+ unrelated.setDaemon(true);
+ unrelated.start();
+ assertTrue(holding.await(10, TimeUnit.SECONDS), "the unrelated thread should hold the interned monitor");
+
+ try {
+ ExecutorService executor = Executors.newSingleThreadExecutor();
Review Comment:
nit, feel free to ignore: this executor plumbing is `assertTimeoutPreemptively`.
L118-135 hand-roll what `Assertions.assertTimeoutPreemptively(Duration, ThrowingSupplier<T>, String)` already does, custom failure message included, and that is the existing idiom for "must not block" in this package (`TestZookeeperBasedLockProvider.java:213`). It is present and not deprecated in the pinned JUnit 5.14.1.
If you take the unlock/close suggestion below, this collapses to two `assertTimeoutPreemptively` calls and lets you drop the `ExecutorService`, `Future` and `TimeoutException` imports. Entirely optional, the current code is correct as written.
--
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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5349414272
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989",
"triggerID" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"triggerType" : "PUSH"
}, {
"hash" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16029",
"triggerID" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"triggerType" : "PUSH"
}, {
"hash" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16221",
"triggerID" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"triggerType" : "PUSH"
}, {
"hash" : "58219469d783bb39fa562117550fe2682a8e7486",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16395",
"triggerID" : "58219469d783bb39fa562117550fe2682a8e7486",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* a639f7364212bab80b7ccf398a2808976732a7d5 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16221)
* 58219469d783bb39fa562117550fe2682a8e7486 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16395)
<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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "rangareddy (via GitHub)" <gi...@apache.org>.
rangareddy commented on code in PR #19486:
URL: https://github.com/apache/hudi/pull/19486#discussion_r3709859335
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -63,14 +63,30 @@
@Slf4j
public class FileSystemBasedLockProvider implements LockProvider<String>, Serializable {
private static final String LOCK_FILE_NAME = "lock";
+ /**
+ * Guards this provider's lock-file operations.
+ *
+ * <p>These blocks used to synchronize on {@link #LOCK_FILE_NAME}. That is a compile-time String
+ * constant, so it is interned and shared JVM-wide with every other {@code "lock"} literal - including
+ * {@code FileSystemBasedLockProviderTestClass}, which declares its own {@code static final String LOCK =
Review Comment:
Applied. The javadoc now states the mechanism without naming anything that can be renamed:
```java
* <p>These blocks used to synchronize on {@link #LOCK_FILE_NAME}. That is a compile-time String constant,
* so it is interned: any class anywhere in the JVM that synchronizes on the same {@code "lock"} literal
* contends on the very same monitor and silently couples itself to Hudi's lock acquisition. A private
* object cannot be aliased that way.
```
The concrete instance stays in the commit message and the PR description, where it is evidence that the hazard is real rather than theoretical, and where going stale costs nothing. Agreed that a production comment which silently rots on a test rename is the wrong place for it.
--
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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5165485914
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989",
"triggerID" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 1556e0d14f3d913b9c9c5381e3b69473e3741902 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989)
<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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "rangareddy (via GitHub)" <gi...@apache.org>.
rangareddy commented on code in PR #19486:
URL: https://github.com/apache/hudi/pull/19486#discussion_r3817547088
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -63,14 +63,29 @@
@Slf4j
public class FileSystemBasedLockProvider implements LockProvider<String>, Serializable {
private static final String LOCK_FILE_NAME = "lock";
+ /**
+ * Guards this provider's lock-file operations.
+ *
+ * <p>These blocks used to synchronize on {@link #LOCK_FILE_NAME}. That is a compile-time String constant,
+ * so it is interned: any class anywhere in the JVM that synchronizes on the same {@code "lock"} literal
+ * contends on the very same monitor and silently couples itself to Hudi's lock acquisition. A private
+ * object cannot be aliased that way.
+ *
+ * <p>Kept static so the mutual-exclusion scope is unchanged by this fix.
Review Comment:
Applied your suggestion verbatim. Agreed the old wording overclaimed.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -63,14 +63,29 @@
@Slf4j
public class FileSystemBasedLockProvider implements LockProvider<String>, Serializable {
private static final String LOCK_FILE_NAME = "lock";
+ /**
+ * Guards this provider's lock-file operations.
+ *
+ * <p>These blocks used to synchronize on {@link #LOCK_FILE_NAME}. That is a compile-time String constant,
+ * so it is interned: any class anywhere in the JVM that synchronizes on the same {@code "lock"} literal
+ * contends on the very same monitor and silently couples itself to Hudi's lock acquisition. A private
+ * object cannot be aliased that way.
+ *
+ * <p>Kept static so the mutual-exclusion scope is unchanged by this fix.
+ */
+ private static final Object LOCK_FILE_MONITOR = new Object();
Review Comment:
Fixed, the trailer now reads `Part of #16943` so the issue stays open for the remaining providers and the close-during-unlock semantics.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -63,14 +63,29 @@
@Slf4j
public class FileSystemBasedLockProvider implements LockProvider<String>, Serializable {
private static final String LOCK_FILE_NAME = "lock";
+ /**
+ * Guards this provider's lock-file operations.
+ *
+ * <p>These blocks used to synchronize on {@link #LOCK_FILE_NAME}. That is a compile-time String constant,
+ * so it is interned: any class anywhere in the JVM that synchronizes on the same {@code "lock"} literal
+ * contends on the very same monitor and silently couples itself to Hudi's lock acquisition. A private
+ * object cannot be aliased that way.
+ *
+ * <p>Kept static so the mutual-exclusion scope is unchanged by this fix.
+ */
+ private static final Object LOCK_FILE_MONITOR = new Object();
private final int lockTimeoutMinutes;
private final transient HoodieStorage storage;
private final transient StoragePath lockFile;
protected LockConfiguration lockConfiguration;
private final SimpleDateFormat sdf;
private final LockInfo lockInfo;
+ /**
+ * Written while holding {@link #LOCK_FILE_MONITOR} in {@code tryLock}, but read through the generated
+ * getter without it, so the read needs to be volatile for the value to be visible to other threads.
+ */
Review Comment:
Reworded to the public-API reason, since you are right that both in-repo readers are on the calling thread. Kept the modifier. Added the `serialVersionUID` change to the description with the two measured values.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -63,14 +63,29 @@
@Slf4j
public class FileSystemBasedLockProvider implements LockProvider<String>, Serializable {
private static final String LOCK_FILE_NAME = "lock";
+ /**
+ * Guards this provider's lock-file operations.
+ *
+ * <p>These blocks used to synchronize on {@link #LOCK_FILE_NAME}. That is a compile-time String constant,
+ * so it is interned: any class anywhere in the JVM that synchronizes on the same {@code "lock"} literal
+ * contends on the very same monitor and silently couples itself to Hudi's lock acquisition. A private
+ * object cannot be aliased that way.
+ *
+ * <p>Kept static so the mutual-exclusion scope is unchanged by this fix.
+ */
+ private static final Object LOCK_FILE_MONITOR = new Object();
private final int lockTimeoutMinutes;
private final transient HoodieStorage storage;
private final transient StoragePath lockFile;
protected LockConfiguration lockConfiguration;
private final SimpleDateFormat sdf;
private final LockInfo lockInfo;
+ /**
+ * Written while holding {@link #LOCK_FILE_MONITOR} in {@code tryLock}, but read through the generated
+ * getter without it, so the read needs to be volatile for the value to be visible to other threads.
+ */
@Getter
- private String currentOwnerLockInfo;
+ private volatile String currentOwnerLockInfo;
Review Comment:
All three corrected in the description. Verified locally: `DynamoDBBasedLockProviderBase:80` is `protected volatile LockItem lock`, so the contrast I drew was wrong, and the description now says so explicitly rather than quietly dropping it. Also folded in the expiry-path double-delete as the reason the monitor has to stay static.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/lock/FileSystemBasedLockProvider.java:
##########
@@ -120,7 +135,7 @@ public void close() {
@Override
public boolean tryLock(long time, TimeUnit unit) {
try {
- synchronized (LOCK_FILE_NAME) {
+ synchronized (LOCK_FILE_MONITOR) {
Review Comment:
Fixed here rather than deferring, using your version. Added `testReloadCurrentOwnerLockInfoClearsWhenLockFileIsGone`, which fails against the old method with `FileNotFoundException` out of the reload.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestFileSystemBasedLockProvider.java:
##########
@@ -81,6 +86,60 @@ public void testAcquireAndReleaseLock() {
}
}
+ /**
+ * The lock-file operations used to synchronize on the {@code "lock"} String constant, which is interned
+ * and therefore shared JVM-wide with every other {@code "lock"} literal. Unrelated code holding that
+ * monitor blocked lock acquisition outright. {@code FileSystemBasedLockProviderTestClass} in this very
+ * repo declares {@code static final String LOCK = "lock"} and so aliases it.
Review Comment:
Rewritten as `{@link}`. The sentence also had to change: the helper no longer aliases the literal after this revision, so it now reads as historical.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestFileSystemBasedLockProvider.java:
##########
@@ -81,6 +86,60 @@ public void testAcquireAndReleaseLock() {
}
}
+ /**
+ * The lock-file operations used to synchronize on the {@code "lock"} String constant, which is interned
+ * and therefore shared JVM-wide with every other {@code "lock"} literal. Unrelated code holding that
+ * monitor blocked lock acquisition outright. {@code FileSystemBasedLockProviderTestClass} in this very
+ * repo declares {@code static final String LOCK = "lock"} and so aliases it.
+ */
+ @Test
+ public void testAcquisitionIsNotBlockedByTheInternedLockLiteral() throws Exception {
+ StorageConfiguration<?> storageConf = HoodieTestUtils.getDefaultStorageConf();
+ FileSystemBasedLockProvider provider =
+ new FileSystemBasedLockProvider(lockConfiguration(lockDir("interned"), 0), storageConf);
+ CountDownLatch holding = new CountDownLatch(1);
+ CountDownLatch release = new CountDownLatch(1);
+ // stands in for any other class in the JVM doing synchronized ("lock")
+ Thread unrelated = new Thread(() -> {
+ synchronized ("lock") {
Review Comment:
Folded in. `LOCK` is split into a private `Object` monitor and a `LOCK_FILE_NAME` string, so there is no `"lock"` monitor left in the tree. No `notify`/`notifyAll` anywhere and `wait()` still targets the synchronized object, so it is monitor-for-monitor.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestFileSystemBasedLockProvider.java:
##########
@@ -81,6 +86,60 @@ public void testAcquireAndReleaseLock() {
}
}
+ /**
+ * The lock-file operations used to synchronize on the {@code "lock"} String constant, which is interned
+ * and therefore shared JVM-wide with every other {@code "lock"} literal. Unrelated code holding that
+ * monitor blocked lock acquisition outright. {@code FileSystemBasedLockProviderTestClass} in this very
+ * repo declares {@code static final String LOCK = "lock"} and so aliases it.
+ */
+ @Test
+ public void testAcquisitionIsNotBlockedByTheInternedLockLiteral() throws Exception {
+ StorageConfiguration<?> storageConf = HoodieTestUtils.getDefaultStorageConf();
+ FileSystemBasedLockProvider provider =
+ new FileSystemBasedLockProvider(lockConfiguration(lockDir("interned"), 0), storageConf);
+ CountDownLatch holding = new CountDownLatch(1);
+ CountDownLatch release = new CountDownLatch(1);
+ // stands in for any other class in the JVM doing synchronized ("lock")
+ Thread unrelated = new Thread(() -> {
+ synchronized ("lock") {
+ holding.countDown();
+ try {
+ release.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ });
+ unrelated.setDaemon(true);
+ unrelated.start();
+ assertTrue(holding.await(10, TimeUnit.SECONDS), "the unrelated thread should hold the interned monitor");
+
+ try {
Review Comment:
Applied. `start()` and the await are inside the try, so the latch always drops.
--
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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5252231521
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989",
"triggerID" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"triggerType" : "PUSH"
}, {
"hash" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16029",
"triggerID" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"triggerType" : "PUSH"
}, {
"hash" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 9d0b7756bff92dd510601481a1397fab6a3a57a1 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16029)
* a639f7364212bab80b7ccf398a2808976732a7d5 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): stop guarding the filesystem lock with an interned string literal and make the provider serializable [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5372950177
Pushed three commits addressing review rounds 2 through 5 (the later rounds reviewed the fixes themselves, on two different models); the PR description has been updated to match the final diff, including a squash message for the merging committer.
`771e03fa09a9`:
- **The exclusive-mode create is now pinned.** `storage.create(path, false)` in `acquireLock` is the provider's cross-process mutual exclusion, and no test anywhere exercised its already-exists arm; flipping the `false` to `true` left everything green. Given `5faefcd01fa8` broke the guarded create for ~2.5 years unnoticed, that gap mattered. `acquireLock` is package-private `@VisibleForTesting`; the loser now fails on `FileAlreadyExistsException` with the winner's payload intact, and `tryLock`'s catch arm (the clean `false` a losing writer sees) is reached deterministically by pointing the lock directory at a regular file. The javadocs say "exclusive-mode create", not "atomic": HDFS guarantees atomicity there, local FS's create is check-then-act.
- **The reload fix closed one race window and left the mirror one open.** Checking `exists()` before `open()` handles an already-gone lock file, but a file deleted between the two calls still threw, reproducing the stale-owner symptom this PR fixes. The `exists()` check is gone; `FileNotFoundException` clears the field, pinned via a `hoodie.storage.class` stub whose `exists()` lies while `open()` throws, and `HoodieStorage.open` documents the contract.
- **The class could never actually serialize.** `lockInfo` is built eagerly and `LockInfo` is not `Serializable` (since HUDI-5377), and the marker is load-bearing: `CleanActionExecutor`'s Spark closure captures the transaction manager and with it the provider, the HUDI-7782 bug class. `lockInfo`/`sdf` are transient now, `serialVersionUID` pinned at 1L (compat-safe: 0.12.x streams carried a computed UID no 0.13.0+ build could read anyway), round trip covered by test.
`428a7197aee0` + `831aed976cbf` (the latter also corrects two claims in the former's message; the description carries the definitive squash message):
- **The FNFE-vs-IOException split is pinned on both sides**: a non-missing-file IO error escapes as `HoodieIOException` and keeps the last known owner rather than clearing it.
- **`checkIfExpired`'s catch had never executed.** A stub failing only `getPathInfo` pins that a stat failure cannot steal a live lock (the holder's on-disk payload survives byte-identical) and cannot skip the owner-info reload. The fail-safe-toward-the-holder choice matches `StorageBasedLockProvider`'s `UNKNOWN_ERROR` arm and is documented; the catch also logged `ALREADY_RELEASED`, the opposite of what it concludes, and logs `ACQUIRING` now.
- `initLockInfo` is synchronized (the lazy fields lost `final`'s free safe publication); `currentOwnerLockInfo` initializes to `""` matching the `LockProvider` interface default, so `LockManager` never logs a `null` owner.
Every pin was reverted individually to confirm it fails exactly the assertion written for it. Transaction package: 262 tests, 0 failures; checkstyle and rat clean on both touched modules. The serialVersionUID paragraph in the description should be replaced by a note that the class has not been Java-serializable at all since 0.13.0.
--
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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "rangareddy (via GitHub)" <gi...@apache.org>.
rangareddy commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5252115778
Squashed to a single commit and rebased onto current master (31 commits of drift; neither file had been touched upstream, so no conflicts). Re-ran everything after the rebase rather than assuming it still held: `org.apache.hudi.client.transaction.**` 256 tests green, checkstyle 0, apache-rat 0.
Two changes while reviewing, both in the test:
1. The regression previously failed with a bare `java.util.concurrent.TimeoutException` from `Future.get`, which tells a reader nothing. It now converts that into an `AssertionError` naming the cause:
```
java.lang.AssertionError: tryLock never returned - it is blocked on the monitor held by the unrelated
thread, which means the provider is synchronizing on the interned "lock" literal again rather than on a
private monitor
```
Re-verified both arms afterwards: production file reverted to master -> 10 run / 1 failed with that message; with the fix -> 10 run / 0 failed.
2. The evidence for the hazard is sharper than the description claimed, so I strengthened it: `FileSystemBasedLockProviderTestClass` does not merely synchronize on its `LOCK = "lock"` constant, it calls `LOCK.wait(retryWaitTimeMs)` inside that block (line 79). An unrelated provider implementation is therefore waiting and notifying on the very monitor this provider takes.
The description is updated for both, including the failure transcript, which quoted the old `TimeoutException`. I also unchecked "CI passes on my PR" — the checks are queued, and the module evidence above is local.
@hudi-agent's nit from the first round (do not name a test class in production Javadoc) remains applied: the production comment states the mechanism only, and the concrete instance lives here and in the commit message where going stale costs nothing.
--
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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5165435150
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 1556e0d14f3d913b9c9c5381e3b69473e3741902 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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5175399448
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989",
"triggerID" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"triggerType" : "PUSH"
}, {
"hash" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16029",
"triggerID" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 1556e0d14f3d913b9c9c5381e3b69473e3741902 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989)
* 9d0b7756bff92dd510601481a1397fab6a3a57a1 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16029)
<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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5350029430
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989",
"triggerID" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"triggerType" : "PUSH"
}, {
"hash" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16029",
"triggerID" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"triggerType" : "PUSH"
}, {
"hash" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16221",
"triggerID" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"triggerType" : "PUSH"
}, {
"hash" : "58219469d783bb39fa562117550fe2682a8e7486",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16395",
"triggerID" : "58219469d783bb39fa562117550fe2682a8e7486",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 58219469d783bb39fa562117550fe2682a8e7486 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16395)
<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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "rangareddy (via GitHub)" <gi...@apache.org>.
rangareddy commented on code in PR #19486:
URL: https://github.com/apache/hudi/pull/19486#discussion_r3817548188
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestFileSystemBasedLockProvider.java:
##########
@@ -81,6 +86,60 @@ public void testAcquireAndReleaseLock() {
}
}
+ /**
+ * The lock-file operations used to synchronize on the {@code "lock"} String constant, which is interned
+ * and therefore shared JVM-wide with every other {@code "lock"} literal. Unrelated code holding that
+ * monitor blocked lock acquisition outright. {@code FileSystemBasedLockProviderTestClass} in this very
+ * repo declares {@code static final String LOCK = "lock"} and so aliases it.
+ */
+ @Test
+ public void testAcquisitionIsNotBlockedByTheInternedLockLiteral() throws Exception {
+ StorageConfiguration<?> storageConf = HoodieTestUtils.getDefaultStorageConf();
+ FileSystemBasedLockProvider provider =
+ new FileSystemBasedLockProvider(lockConfiguration(lockDir("interned"), 0), storageConf);
+ CountDownLatch holding = new CountDownLatch(1);
+ CountDownLatch release = new CountDownLatch(1);
+ // stands in for any other class in the JVM doing synchronized ("lock")
+ Thread unrelated = new Thread(() -> {
+ synchronized ("lock") {
+ holding.countDown();
+ try {
+ release.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ });
+ unrelated.setDaemon(true);
+ unrelated.start();
+ assertTrue(holding.await(10, TimeUnit.SECONDS), "the unrelated thread should hold the interned monitor");
+
+ try {
+ ExecutorService executor = Executors.newSingleThreadExecutor();
Review Comment:
Taken, together with the unlock/close suggestion. Both checks are `assertTimeoutPreemptively` now and the `ExecutorService`, `Future` and `TimeoutException` imports are gone.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/transaction/lock/TestFileSystemBasedLockProvider.java:
##########
@@ -81,6 +86,60 @@ public void testAcquireAndReleaseLock() {
}
}
+ /**
+ * The lock-file operations used to synchronize on the {@code "lock"} String constant, which is interned
+ * and therefore shared JVM-wide with every other {@code "lock"} literal. Unrelated code holding that
+ * monitor blocked lock acquisition outright. {@code FileSystemBasedLockProviderTestClass} in this very
+ * repo declares {@code static final String LOCK = "lock"} and so aliases it.
+ */
+ @Test
+ public void testAcquisitionIsNotBlockedByTheInternedLockLiteral() throws Exception {
+ StorageConfiguration<?> storageConf = HoodieTestUtils.getDefaultStorageConf();
+ FileSystemBasedLockProvider provider =
+ new FileSystemBasedLockProvider(lockConfiguration(lockDir("interned"), 0), storageConf);
+ CountDownLatch holding = new CountDownLatch(1);
+ CountDownLatch release = new CountDownLatch(1);
+ // stands in for any other class in the JVM doing synchronized ("lock")
+ Thread unrelated = new Thread(() -> {
+ synchronized ("lock") {
+ holding.countDown();
+ try {
+ release.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ });
+ unrelated.setDaemon(true);
+ unrelated.start();
+ assertTrue(holding.await(10, TimeUnit.SECONDS), "the unrelated thread should hold the interned monitor");
+
+ try {
+ ExecutorService executor = Executors.newSingleThreadExecutor();
+ try {
+ Future<Boolean> acquired = executor.submit(() -> provider.tryLock(1, TimeUnit.SECONDS));
+ boolean gotLock;
+ try {
+ gotLock = acquired.get(10, TimeUnit.SECONDS);
+ } catch (TimeoutException e) {
+ // The bare TimeoutException says nothing about why, and this is the failure the regression
+ // produces: tryLock is parked on a monitor an unrelated thread holds, so it never returns.
+ throw new AssertionError("tryLock never returned - it is blocked on the monitor held by the "
+ + "unrelated thread, which means the provider is synchronizing on the interned \"lock\" "
+ + "literal again rather than on a private monitor", e);
+ }
+ assertTrue(gotLock,
+ "acquisition must not wait on a monitor held by code that has nothing to do with Hudi");
+ } finally {
+ executor.shutdownNow();
+ }
+ } finally {
+ release.countDown();
+ provider.unlock();
+ provider.close();
Review Comment:
Good catch, that was the real gap. Both are now exercised while the monitor is held. Confirmed it discriminates by reverting each block on its own: `close()`, `unlock()` and `tryLock()` each fail the test individually now, where before only `tryLock` did.
--
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): stop guarding the filesystem lock with an interned string literal [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19486:
URL: https://github.com/apache/hudi/pull/19486#issuecomment-5372837886
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15989",
"triggerID" : "1556e0d14f3d913b9c9c5381e3b69473e3741902",
"triggerType" : "PUSH"
}, {
"hash" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16029",
"triggerID" : "9d0b7756bff92dd510601481a1397fab6a3a57a1",
"triggerType" : "PUSH"
}, {
"hash" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16221",
"triggerID" : "a639f7364212bab80b7ccf398a2808976732a7d5",
"triggerType" : "PUSH"
}, {
"hash" : "58219469d783bb39fa562117550fe2682a8e7486",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16395",
"triggerID" : "58219469d783bb39fa562117550fe2682a8e7486",
"triggerType" : "PUSH"
}, {
"hash" : "771e03fa09a94f284889d9089ccbbd7cd5f946f7",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16465",
"triggerID" : "771e03fa09a94f284889d9089ccbbd7cd5f946f7",
"triggerType" : "PUSH"
}, {
"hash" : "428a7197aee0cd5114ef21112448d838c80ad9da",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16467",
"triggerID" : "428a7197aee0cd5114ef21112448d838c80ad9da",
"triggerType" : "PUSH"
}, {
"hash" : "831aed976cbfaa8dcb64f8f9c490aa95d86494f9",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "831aed976cbfaa8dcb64f8f9c490aa95d86494f9",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 58219469d783bb39fa562117550fe2682a8e7486 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16395)
* 771e03fa09a94f284889d9089ccbbd7cd5f946f7 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16465)
* 428a7197aee0cd5114ef21112448d838c80ad9da Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16467)
* 831aed976cbfaa8dcb64f8f9c490aa95d86494f9 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