yuqi1129 commented on code in PR #12417:
URL: https://github.com/apache/gravitino/pull/12417#discussion_r3763039733
##########
core/src/main/java/org/apache/gravitino/cache/CaffeineEntityCache.java:
##########
@@ -83,6 +85,13 @@ public class CaffeineEntityCache extends BaseEntityCache {
private static final Logger LOG =
LoggerFactory.getLogger(CaffeineEntityCache.class.getName());
+ /**
+ * Characters that can separate a cached entity's identifier from a
descendant's. See {@link
+ * #invalidateHierarchy(EntityCacheKey)} for why both are needed.
+ */
+ private static final List<String> CHILD_KEY_BOUNDARIES =
+ ImmutableList.of(".", HierarchicalSchemaUtil.physicalSeparator());
Review Comment:
Yes — you are right, and it was a real defect, not a style point.
The cache lives in `RelationalEntityStore`, above the PO layer where
`SchemaMetaService` runs the logical<->physical conversion
(`HierarchicalConversionPOStorageOps`), so cached identifiers still carry the
configured external separator. `\u0001` only exists in backend rows. Scanning
for the physical separator matched nothing in production; the unit tests only
passed because they built their names with the physical separator too.
These are the real cache keys dumped from an H2-backed
`RelationalEntityStore` (default separator, then `|`):
```
metalake.catalog.raw:events:SCHEMA
metalake.catalog.raw|events:SCHEMA
metalake.catalog.raw:events:2024:SCHEMA
metalake.catalog.raw|events|2024:SCHEMA
metalake.catalog.raw:events:2024.t_child:TABLE
metalake.catalog.raw|events|2024.t_child:TABLE
```
Fixed to use `HierarchicalSchemaUtil.schemaSeparator()`. The static
`CHILD_KEY_BOUNDARIES` constant is gone as well, since the separator is
config-derived and must be read per call.
One consequence found while fixing this: `EntityCacheKey.toString()` is
`"<identifier>:<type>"`, and `":"` is also the default separator, so an
unguarded second scan over a table key would also evict the topic/fileset of
the same name. Only a schema can nest, and a catalog reaches its nested schemas
through the `"."` pass, so the second pass now runs for schema keys only, with
a regression test (`testInvalidateLeafDoesNotEvictSameNameEntityOfAnotherType`).
##########
core/src/main/java/org/apache/gravitino/cache/CaffeineEntityCache.java:
##########
@@ -267,22 +276,36 @@ protected void invalidateExpiredItem(EntityCacheKey key) {
/**
* Removes the entry for the given key and all cached descendant entries.
Descendants are found
- * through the prefix index: every child identifier starts with {@code
parent identifier + "."},
- * so the scan is exact for children and never matches siblings sharing a
name prefix (e.g. {@code
- * catalog1} vs {@code catalog10}).
+ * through the prefix index, scanning once per child boundary:
+ *
+ * <ul>
+ * <li>{@code "."} separates {@link org.apache.gravitino.NameIdentifier}
levels, so it matches
Review Comment:
Fixed: `{@link org.apache.gravitino.NameIdentifier}` -> `{@link
NameIdentifier}`, the class is already imported.
##########
core/src/test/java/org/apache/gravitino/cache/TestCaffeineEntityCacheInvalidation.java:
##########
@@ -155,6 +167,108 @@ void testGetIfPresentReturnsCachedEntity() {
cache.getIfPresent(catalog.nameIdentifier(),
Entity.EntityType.SCHEMA).isEmpty());
}
+ @Test
+ void testInvalidateHierarchicalSchemaCascadesToNestedSchemas() {
+ // A HierarchicalSchema nests inside a single name level, joined by the
physical separator,
+ // so "raw:events:2024" is a child of "raw:events" without adding a
NameIdentifier level.
+ String parentName = hierarchicalName("raw", "events");
+ String childName = hierarchicalName("raw", "events", "2024");
+
+ SchemaEntity parent = TestUtil.getTestSchemaEntity(2L, parentName,
CATALOG_NS, "cmt");
+ SchemaEntity child = TestUtil.getTestSchemaEntity(3L, childName,
CATALOG_NS, "cmt");
+ TableEntity tableInParent =
+ TestUtil.getTestTableEntity(4L, "t_parent",
schemaNamespace(parentName));
+ TableEntity tableInChild =
+ TestUtil.getTestTableEntity(5L, "t_child", schemaNamespace(childName));
+
+ cache.put(parent);
+ cache.put(child);
+ cache.put(tableInParent);
+ cache.put(tableInChild);
+ Assertions.assertEquals(4, cache.size());
+
+ cache.invalidate(parent.nameIdentifier(), Entity.EntityType.SCHEMA);
+
+ Assertions.assertFalse(cache.contains(parent.nameIdentifier(),
Entity.EntityType.SCHEMA));
+ Assertions.assertFalse(cache.contains(tableInParent.nameIdentifier(),
Entity.EntityType.TABLE));
+ Assertions.assertFalse(cache.contains(child.nameIdentifier(),
Entity.EntityType.SCHEMA));
+ Assertions.assertFalse(cache.contains(tableInChild.nameIdentifier(),
Entity.EntityType.TABLE));
+ Assertions.assertEquals(0, cache.size());
+ }
+
+ @Test
+ void testInvalidateHierarchicalSchemaCascadesToAnyDepth() {
+ String level1 = hierarchicalName("raw");
+ String level2 = hierarchicalName("raw", "events");
+ String level3 = hierarchicalName("raw", "events", "2024");
+ String level4 = hierarchicalName("raw", "events", "2024", "q1");
+
+ cache.put(TestUtil.getTestSchemaEntity(2L, level1, CATALOG_NS, "cmt"));
+ cache.put(TestUtil.getTestSchemaEntity(3L, level2, CATALOG_NS, "cmt"));
+ cache.put(TestUtil.getTestSchemaEntity(4L, level3, CATALOG_NS, "cmt"));
+ SchemaEntity deepest = TestUtil.getTestSchemaEntity(5L, level4,
CATALOG_NS, "cmt");
+ cache.put(deepest);
+ TableEntity deepestTable = TestUtil.getTestTableEntity(6L, "t_deep",
schemaNamespace(level4));
+ cache.put(deepestTable);
+ Assertions.assertEquals(5, cache.size());
+
+ cache.invalidate(
+ TestUtil.getTestSchemaEntity(2L, level1, CATALOG_NS,
"cmt").nameIdentifier(),
+ Entity.EntityType.SCHEMA);
+
+ Assertions.assertFalse(cache.contains(deepest.nameIdentifier(),
Entity.EntityType.SCHEMA));
+ Assertions.assertFalse(cache.contains(deepestTable.nameIdentifier(),
Entity.EntityType.TABLE));
+ Assertions.assertEquals(0, cache.size());
+ }
+
+ @Test
+ void testInvalidateHierarchicalSchemaDoesNotTouchSiblings() {
+ // Guards against over-matching: "raw:events2" is a sibling of
"raw:events", not a descendant,
+ // exactly like the catalog1 / catalog10 case the "." boundary already
protects against.
+ String target = hierarchicalName("raw", "events");
+ String sibling = hierarchicalName("raw", "events2");
+ String siblingOfParent = hierarchicalName("raw2", "events");
+
+ SchemaEntity targetSchema = TestUtil.getTestSchemaEntity(2L, target,
CATALOG_NS, "cmt");
+ SchemaEntity siblingSchema = TestUtil.getTestSchemaEntity(3L, sibling,
CATALOG_NS, "cmt");
+ SchemaEntity otherBranch = TestUtil.getTestSchemaEntity(4L,
siblingOfParent, CATALOG_NS, "cmt");
+ TableEntity siblingTable =
+ TestUtil.getTestTableEntity(5L, "t_sibling", schemaNamespace(sibling));
+
+ cache.put(targetSchema);
+ cache.put(siblingSchema);
+ cache.put(otherBranch);
+ cache.put(siblingTable);
+
+ cache.invalidate(targetSchema.nameIdentifier(), Entity.EntityType.SCHEMA);
+
+ Assertions.assertFalse(cache.contains(targetSchema.nameIdentifier(),
Entity.EntityType.SCHEMA));
+ Assertions.assertTrue(cache.contains(siblingSchema.nameIdentifier(),
Entity.EntityType.SCHEMA));
+ Assertions.assertTrue(cache.contains(otherBranch.nameIdentifier(),
Entity.EntityType.SCHEMA));
+ Assertions.assertTrue(cache.contains(siblingTable.nameIdentifier(),
Entity.EntityType.TABLE));
+ Assertions.assertEquals(3, cache.size());
+ }
+
+ @Test
+ void testInvalidateCatalogCascadesToHierarchicalSchemas() {
+ CatalogEntity catalog =
+ TestUtil.getTestCatalogEntity(1L, "catalog1",
Namespace.of("metalake"), "hive", "cmt");
+ String nested = hierarchicalName("raw", "events", "2024");
+ SchemaEntity schema = TestUtil.getTestSchemaEntity(2L, nested, CATALOG_NS,
"cmt");
+ TableEntity table = TestUtil.getTestTableEntity(3L, "t1",
schemaNamespace(nested));
+
+ cache.put(catalog);
+ cache.put(schema);
+ cache.put(table);
+ Assertions.assertEquals(3, cache.size());
+
+ cache.invalidate(catalog.nameIdentifier(), Entity.EntityType.CATALOG);
+
+ Assertions.assertFalse(cache.contains(schema.nameIdentifier(),
Entity.EntityType.SCHEMA));
+ Assertions.assertFalse(cache.contains(table.nameIdentifier(),
Entity.EntityType.TABLE));
+ Assertions.assertEquals(0, cache.size());
+ }
+
Review Comment:
Both added.
`TestRelationalEntityStoreHierarchicalCache` drives a real H2-backed
`RelationalEntityStore` with the cache enabled, parameterized over the default
separator `:` and a non-default `|`. It writes `raw:events`, the nested
`raw:events:2024`, a table inside the nested schema and the sibling
`raw:events2`, reads them back through the store, drops `raw:events` with
cascade, then asserts the nested schema and its table are evicted while the
sibling survives. It also asserts that no cache key contains the physical
separator, which is what pinned down the wrong boundary in the first place.
Both parameter sets fail before the fix.
`TestCaffeineEntityCacheInvalidation` gets the cache-level counterpart,
`testInvalidateHierarchicalSchemaCascadesWithNonDefaultSeparator` (separator
`|`), which also fails before the fix.
The store-level test runs against H2 by default; MySQL/PostgreSQL come from
CI.
--
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]
