Copilot commented on code in PR #13400:
URL: https://github.com/apache/gravitino/pull/13400#discussion_r4069188691
##########
core/src/main/java/org/apache/gravitino/cache/SegmentedLock.java:
##########
@@ -34,8 +34,15 @@ public class SegmentedLock {
private final Striped<Lock> stripedLocks;
- /** CountDownLatch for global operations - null when no operation is in
progress */
- private final AtomicReference<CountDownLatch> globalOperationLatch = new
AtomicReference<>();
+ /**
+ * Gates segment operations against global operations: segment operations
hold the read lock
+ * across their whole critical section, global operations hold the write
lock, so a global
+ * operation excludes every segment operation, including ones already in
flight when it starts.
+ */
+ private final ReentrantReadWriteLock globalGate = new
ReentrantReadWriteLock();
Review Comment:
The gate uses the default non-fair mode, so readers can barge ahead of a
queued writer. Once `withGlobalLock` is waiting for an in-flight reader, a
continuous stream of `withLock` calls can keep acquiring the read lock and
indefinitely starve the global action, leaving cache clears blocked. Use a fair
read/write lock (or otherwise prevent readers from entering once a writer is
queued).
##########
core/src/test/java/org/apache/gravitino/cache/TestSegmentedLock.java:
##########
@@ -380,4 +381,58 @@ void testConcurrentGlobalClearing() {
});
});
}
+
+ @Test
+ @Timeout(30)
+ void testGlobalClearingWaitsForInFlightOperations() throws
InterruptedException {
+ SegmentedLock lock = new SegmentedLock(4);
+ CountDownLatch insideSegmentOp = new CountDownLatch(1);
+ CountDownLatch releaseSegmentOp = new CountDownLatch(1);
+ CountDownLatch globalActionDone = new CountDownLatch(1);
+ AtomicBoolean overlapped = new AtomicBoolean(false);
+
+ Thread segmentOpThread =
+ new Thread(
+ () ->
+ lock.withLock(
+ "key1",
+ () -> {
+ insideSegmentOp.countDown();
+ try {
+ releaseSegmentOp.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }));
+ segmentOpThread.start();
+ assertTrue(insideSegmentOp.await(5, TimeUnit.SECONDS), "segment operation
never started");
+
+ Thread globalThread =
+ new Thread(
+ () ->
+ lock.withGlobalLock(
+ () -> {
+ // The global action must never observe the in-flight
segment
+ // operation still holding its critical section.
+ if (releaseSegmentOp.getCount() > 0) {
+ overlapped.set(true);
+ }
Review Comment:
`releaseSegmentOp` only signals that the segment action may return; it is
counted down before the lambda returns and before `withLock` releases its
segment/read locks. A regression that releases the global gate before the
action exits could therefore run the global action after this latch is released
and still pass. Add a separate completion latch counted down after the segment
action finishes, and assert that latch in the global action to verify exclusion
across the full critical section.
--
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]
