This is an automated email from the ASF dual-hosted git repository.
jerryshao pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new c49205be57 [#13462] fix(cache): reject nested clear that hangs core
tests (#13463)
c49205be57 is described below
commit c49205be572f62149222e00cd45167f1ff95a451
Author: Qi Yu <[email protected]>
AuthorDate: Wed Sep 23 14:26:31 2026 +0800
[#13462] fix(cache): reject nested clear that hangs core tests (#13463)
### What changes were proposed in this pull request?
Replace the batch-get test's nested cache clear with an unrelated-key
invalidation, which still exercises the post-put epoch check. Make
`SegmentedLock.withGlobalLock` reject a call from a thread holding its
segment read lock, add a regression test, and correct the write-back
comments.
### Why are the changes needed?
Since #13400, a segment operation holds a read lock until it finishes. A
clear invoked from that operation cannot upgrade to the global write
lock and hangs indefinitely. The test introduced in #13374 made exactly
that call. Production code now fails fast if the same invalid call is
made in the future; the revised test covers an invalidation that can
occur during write-back.
Fix: #13462
### Does this PR introduce _any_ user-facing change?
No new API or configuration. An invalid nested cache clear now throws
`IllegalStateException` instead of hanging.
### How was this patch tested?
- `./gradlew :core:test --tests 'org.apache.gravitino.cache.*' --tests
'org.apache.gravitino.storage.relational.TestRelationalEntityStore*'
--tests 'org.apache.gravitino.storage.relational.TestEntityCache*'
--tests 'org.apache.gravitino.storage.relational.TestEntityChangeLog*'
-PskipITs` — 142 tests passed.
- `./gradlew :core:spotlessCheck -PskipITs` — passed.
---
.../org/apache/gravitino/cache/SegmentedLock.java | 9 ++++++++-
.../storage/relational/RelationalEntityStore.java | 4 ++--
.../apache/gravitino/cache/TestSegmentedLock.java | 22 ++++++++++++++++++++++
.../TestRelationalEntityStoreBatchGetLateFill.java | 17 +++++++++++++++--
4 files changed, 47 insertions(+), 5 deletions(-)
diff --git a/core/src/main/java/org/apache/gravitino/cache/SegmentedLock.java
b/core/src/main/java/org/apache/gravitino/cache/SegmentedLock.java
index b6b2d5a8dc..fa4f52daa2 100644
--- a/core/src/main/java/org/apache/gravitino/cache/SegmentedLock.java
+++ b/core/src/main/java/org/apache/gravitino/cache/SegmentedLock.java
@@ -231,11 +231,18 @@ public class SegmentedLock {
* across their whole critical section, and the write lock acquired here
waits for all of them.
*
* <p>Must not be called from inside a {@code withLock} action on the same
instance: the read lock
- * cannot upgrade to the write lock, so such a call would deadlock.
+ * cannot upgrade to the write lock. Such a call fails immediately instead
of deadlocking.
*
* @param action The clearing action to execute
+ * @throws IllegalStateException if the current thread holds this lock's
read lock or another
+ * global operation is in progress
*/
public void withGlobalLock(Runnable action) {
+ if (globalGate.getReadHoldCount() > 0) {
+ throw new IllegalStateException(
+ "Cannot start a global operation while holding a segment lock");
+ }
+
// Mark the global operation in progress, fail if another one is already
running
if (!clearing.compareAndSet(false, true)) {
throw new IllegalStateException("Global operation already in progress");
diff --git
a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java
b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java
index eda8be4d96..748518fbda 100644
---
a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java
+++
b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java
@@ -286,8 +286,8 @@ public class RelationalEntityStore
() -> {
if (cacheInvalidationEpoch.get() == epochBeforeRead) {
cache.put(entity);
- // A whole-cache clear can run while this key lock is held. If
it happened during
- // put, remove the value we may have written after the clear.
+ // Invalidation of another key or an ancestor can advance the
epoch while this key
+ // lock is held. Remove the value if that happened during the
put.
if (cacheInvalidationEpoch.get() != epochBeforeRead) {
cache.invalidate(entity.nameIdentifier(), entity.type());
}
diff --git
a/core/src/test/java/org/apache/gravitino/cache/TestSegmentedLock.java
b/core/src/test/java/org/apache/gravitino/cache/TestSegmentedLock.java
index edce98096a..483520176e 100644
--- a/core/src/test/java/org/apache/gravitino/cache/TestSegmentedLock.java
+++ b/core/src/test/java/org/apache/gravitino/cache/TestSegmentedLock.java
@@ -382,6 +382,28 @@ public class TestSegmentedLock {
});
}
+ @Test
+ @Timeout(5)
+ void testGlobalClearingFromSegmentOperationFailsWithoutPoisoningLock() {
+ SegmentedLock lock = new SegmentedLock(4);
+ AtomicBoolean globalActionRan = new AtomicBoolean(false);
+
+ lock.withLock(
+ "key",
+ () -> {
+ IllegalStateException exception =
+ assertThrows(
+ IllegalStateException.class,
+ () -> lock.withGlobalLock(() -> globalActionRan.set(true)));
+ assertTrue(exception.getMessage().contains("segment lock"));
+ assertFalse(lock.isClearing());
+ });
+
+ assertFalse(globalActionRan.get());
+ assertDoesNotThrow(() -> lock.withGlobalLock(() ->
globalActionRan.set(true)));
+ assertTrue(globalActionRan.get());
+ }
+
@Test
@Timeout(30)
void testGlobalClearingWaitsForInFlightOperations() throws
InterruptedException {
diff --git
a/core/src/test/java/org/apache/gravitino/storage/relational/TestRelationalEntityStoreBatchGetLateFill.java
b/core/src/test/java/org/apache/gravitino/storage/relational/TestRelationalEntityStoreBatchGetLateFill.java
index 8ac5da1689..0a0b8892ab 100644
---
a/core/src/test/java/org/apache/gravitino/storage/relational/TestRelationalEntityStoreBatchGetLateFill.java
+++
b/core/src/test/java/org/apache/gravitino/storage/relational/TestRelationalEntityStoreBatchGetLateFill.java
@@ -21,6 +21,8 @@ package org.apache.gravitino.storage.relational;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
+import java.io.IOException;
+import java.io.UncheckedIOException;
import java.time.Instant;
import java.util.List;
import org.apache.commons.lang3.reflect.FieldUtils;
@@ -159,10 +161,21 @@ public class TestRelationalEntityStoreBatchGetLateFill {
}
@Test
- void testBatchGetRemovesValueWrittenAfterClear() throws
IllegalAccessException {
+ void testBatchGetRemovesValueWrittenAfterInvalidationDuringPut() throws
IllegalAccessException {
TableEntity table = TestUtil.getTestTableEntity(1L, "t1", SCHEMA_NS);
RecordingCache recordingCache = new RecordingCache();
- recordingCache.beforePut = store::clearCache;
+ // Advance the epoch from inside the write-back, while this key's cache
lock is held. Invalidate
+ // an unrelated entity so this entry is removed only by the post-put epoch
check. A whole-cache
+ // clear cannot run from inside the segment operation.
+ NameIdentifier unrelatedIdent = NameIdentifier.of(SCHEMA_NS, "t2");
+ recordingCache.beforePut =
+ () -> {
+ try {
+ store.delete(unrelatedIdent, Entity.EntityType.TABLE, false);
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ };
FieldUtils.writeField(store, "cache", recordingCache, true);
Mockito.when(backend.batchGet(any(),
eq(Entity.EntityType.TABLE))).thenReturn(List.of(table));
