Copilot commented on code in PR #12171:
URL: https://github.com/apache/gravitino/pull/12171#discussion_r3654205959
##########
core/src/test/java/org/apache/gravitino/storage/relational/TestRelationalEntityStore.java:
##########
@@ -228,4 +236,61 @@ void
testUpdateEntityRelationsInvalidatesCacheAfterBackendUpdate()
Entity.EntityType.TABLE,
SupportsRelationOperations.Type.TAG_METADATA_OBJECT_REL);
}
+
+ /**
+ * Verifies that {@link RelationalEntityStore#batchListEntitiesByRelation}
does not hold a cache
+ * lock across the backend call: with the backend blocked mid-call, a
concurrent {@code
+ * invalidate} on the same key must still complete.
+ */
+ @Test
+ void testBatchListDoesNotHoldCacheLockAcrossBackendCall()
+ throws IOException, IllegalAccessException, InterruptedException {
+ SupportsRelationOperations.Type relType =
+ SupportsRelationOperations.Type.TAG_METADATA_OBJECT_REL;
+ Entity.EntityType identType = Entity.EntityType.TABLE;
+ NameIdentifier src = NameIdentifier.of("metalake", "catalog", "schema",
"table1");
+
+ Config config = new Config(false) {};
+ CaffeineEntityCache realCache = new CaffeineEntityCache(config);
+ FieldUtils.writeField(store, "cache", realCache, true);
+
+ CountDownLatch backendEntered = new CountDownLatch(1);
+ CountDownLatch releaseBackend = new CountDownLatch(1);
+ Mockito.when(backend.batchListEntitiesByRelation(eq(relType),
any(List.class), eq(identType)))
+ .thenAnswer(
+ invocation -> {
+ backendEntered.countDown();
+ // Hold the backend "DB" call open to simulate a slow round-trip.
+ releaseBackend.await(10, TimeUnit.SECONDS);
+ return new ArrayList<RelationalEntity<?>>();
Review Comment:
Use the diamond operator here; the explicit generic type is redundant and
adds noise.
##########
core/src/test/java/org/apache/gravitino/storage/relational/TestRelationalEntityStore.java:
##########
@@ -228,4 +236,61 @@ void
testUpdateEntityRelationsInvalidatesCacheAfterBackendUpdate()
Entity.EntityType.TABLE,
SupportsRelationOperations.Type.TAG_METADATA_OBJECT_REL);
}
+
+ /**
+ * Verifies that {@link RelationalEntityStore#batchListEntitiesByRelation}
does not hold a cache
+ * lock across the backend call: with the backend blocked mid-call, a
concurrent {@code
+ * invalidate} on the same key must still complete.
+ */
+ @Test
+ void testBatchListDoesNotHoldCacheLockAcrossBackendCall()
+ throws IOException, IllegalAccessException, InterruptedException {
+ SupportsRelationOperations.Type relType =
+ SupportsRelationOperations.Type.TAG_METADATA_OBJECT_REL;
+ Entity.EntityType identType = Entity.EntityType.TABLE;
+ NameIdentifier src = NameIdentifier.of("metalake", "catalog", "schema",
"table1");
+
+ Config config = new Config(false) {};
+ CaffeineEntityCache realCache = new CaffeineEntityCache(config);
+ FieldUtils.writeField(store, "cache", realCache, true);
+
+ CountDownLatch backendEntered = new CountDownLatch(1);
+ CountDownLatch releaseBackend = new CountDownLatch(1);
+ Mockito.when(backend.batchListEntitiesByRelation(eq(relType),
any(List.class), eq(identType)))
+ .thenAnswer(
+ invocation -> {
+ backendEntered.countDown();
+ // Hold the backend "DB" call open to simulate a slow round-trip.
+ releaseBackend.await(10, TimeUnit.SECONDS);
+ return new ArrayList<RelationalEntity<?>>();
+ });
+
+ ExecutorService executor = Executors.newFixedThreadPool(2);
+ try {
+ Future<List<RelationalEntity<?>>> listFuture =
+ executor.submit(
+ () -> store.batchListEntitiesByRelation(relType, List.of(src),
identType));
+
+ // Wait until the list is blocked inside the backend call.
+ Assertions.assertTrue(
+ backendEntered.await(5, TimeUnit.SECONDS), "backend call should have
been entered");
+
+ // A concurrent cache mutation on the same key must not be blocked by
the in-flight list.
+ Future<Boolean> invalidateFuture =
+ executor.submit(
+ () -> {
+ realCache.invalidate(src, identType, relType);
+ return Boolean.TRUE;
+ });
+ Assertions.assertDoesNotThrow(
+ () -> invalidateFuture.get(5, TimeUnit.SECONDS),
+ "concurrent cache invalidate must not block on the in-flight batch
list");
+
+ releaseBackend.countDown();
+ Assertions.assertDoesNotThrow(() -> listFuture.get(5, TimeUnit.SECONDS));
+ } finally {
+ releaseBackend.countDown();
+ executor.shutdownNow();
+ }
Review Comment:
The test creates a non-daemon ExecutorService; calling shutdownNow() without
awaiting termination can leave threads running if tasks don't exit promptly,
which can make test runs hang or become flaky. Await termination with a bounded
timeout.
--
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]
