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