This is an automated email from the ASF dual-hosted git repository.
yuqi1129 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 0d7e459d60 [#11406] fix(core): reload closed catalog cache wrapper
(#11407)
0d7e459d60 is described below
commit 0d7e459d6026dcdbfe32e07b26766d42cc5febf7
Author: Qi Yu <[email protected]>
AuthorDate: Thu Jun 4 08:57:40 2026 +0800
[#11406] fix(core): reload closed catalog cache wrapper (#11407)
### What changes were proposed in this pull request?
This PR makes `CatalogManager.loadCatalogAndWrap` detect a cached
`CatalogWrapper` whose underlying catalog has already been closed. When
that happens, it invalidates the stale cache entry and reloads the
wrapper from the entity store.
It also adds a regression test that verifies `dropCatalog` can proceed
after the cache contains a closed wrapper.
### Why are the changes needed?
A closed `CatalogWrapper` sets its internal catalog reference to null.
If a later `dropCatalog` call reuses that wrapper from cache, it can
fail with a NullPointerException when checking catalog state or
capabilities.
Fix: #11406
### Does this PR introduce _any_ user-facing change?
No user-facing API or property changes.
### How was this patch tested?
```bash
./gradlew :core:test --tests
org.apache.gravitino.catalog.TestCatalogManager.testDropCatalogReloadsClosedCachedWrapper
-PskipWeb=true -PskipDockerTests=true
./gradlew :core:test --tests
org.apache.gravitino.catalog.TestCatalogManager -PskipWeb=true
-PskipDockerTests=true
git diff --check
```
---
.../apache/gravitino/catalog/CatalogManager.java | 13 ++++-
.../gravitino/catalog/TestCatalogManager.java | 60 ++++++++++++++++++++++
2 files changed, 72 insertions(+), 1 deletion(-)
diff --git
a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
index 203b8c5555..91bd5b08b6 100644
--- a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
+++ b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
@@ -1001,13 +1001,24 @@ public class CatalogManager implements
CatalogDispatcher, Closeable {
/**
* Loads the catalog with the specified identifier, wraps it in a
CatalogWrapper, and caches the
- * wrapper for reuse.
+ * wrapper for reuse. If the cached wrapper has already been closed (its
underlying catalog is
+ * null), the stale entry is evicted and a fresh wrapper is loaded and
cached.
*
* @param ident The identifier of the catalog to load.
* @return The wrapped CatalogWrapper containing the loaded catalog.
* @throws NoSuchCatalogException If the specified catalog does not exist.
*/
public CatalogWrapper loadCatalogAndWrap(NameIdentifier ident) throws
NoSuchCatalogException {
+ CatalogWrapper wrapper = catalogCache.get(ident,
this::loadCatalogInternal);
+ if (wrapper.catalog() != null) {
+ return wrapper;
+ }
+
+ // The cached wrapper has already been closed (catalog() == null), e.g. by
a prior
+ // dropCatalog or cache eviction. Evict the stale entry and reload a fresh
one.
+ // Use a conditional remove so we do not clobber a wrapper that another
thread may
+ // have concurrently reloaded into the cache between our initial get and
this remove.
+ catalogCache.asMap().remove(ident, wrapper);
return catalogCache.get(ident, this::loadCatalogInternal);
}
diff --git
a/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java
b/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java
index 5d978dd3b0..4880d7f258 100644
--- a/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java
+++ b/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java
@@ -1027,6 +1027,66 @@ public class TestCatalogManager {
Assertions.assertNull(catalogManager.getCatalogCache().getIfPresent(ident));
}
+ @Test
+ void testDropCatalogReloadsClosedCachedWrapper() throws Exception {
+ NameIdentifier ident = NameIdentifier.of("metalake",
"closed_cache_drop_test");
+ Map<String, String> props =
+ ImmutableMap.of(
+ "provider",
+ "test",
+ PROPERTY_KEY1,
+ "value1",
+ PROPERTY_KEY2,
+ "value2",
+ PROPERTY_KEY5_PREFIX + "1",
+ "value3");
+
+ Catalog catalog =
+ catalogManager.createCatalog(ident, Catalog.Type.RELATIONAL, provider,
"comment", props);
+ Assertions.assertDoesNotThrow(() -> catalogManager.disableCatalog(ident));
+ CatalogEntity entity = entityStore.get(ident, EntityType.CATALOG,
CatalogEntity.class);
+ FieldUtils.writeField(catalog, "entity", entity, true);
+
+ CatalogManager.CatalogWrapper closedWrapper =
catalogManager.loadCatalogAndWrap(ident);
+ closedWrapper.close();
+ Assertions.assertSame(closedWrapper,
catalogManager.getCatalogCache().getIfPresent(ident));
+
+ boolean dropped = catalogManager.dropCatalog(ident);
+
+ Assertions.assertTrue(dropped);
+ Assertions.assertFalse(entityStore.exists(ident, EntityType.CATALOG));
+
Assertions.assertNull(catalogManager.getCatalogCache().getIfPresent(ident));
+ }
+
+ @Test
+ void testLoadCatalogAndWrapDoesNotInvalidateConcurrentlyReloadedWrapper() {
+ NameIdentifier ident = NameIdentifier.of("metalake",
"concurrent_cache_reload_test");
+
+ CatalogManager.CatalogWrapper closedWrapper =
Mockito.mock(CatalogManager.CatalogWrapper.class);
+ CatalogManager.CatalogWrapper freshWrapper =
Mockito.mock(CatalogManager.CatalogWrapper.class);
+ BaseCatalog<?> freshCatalog = Mockito.mock(BaseCatalog.class);
+ Mockito.doReturn(freshCatalog).when(freshWrapper).catalog();
+ Mockito.doAnswer(
+ invocation -> {
+ catalogManager.getCatalogCache().put(ident, freshWrapper);
+ return null;
+ })
+ .when(closedWrapper)
+ .catalog();
+
+ try {
+ catalogManager.getCatalogCache().put(ident, closedWrapper);
+
+ CatalogManager.CatalogWrapper loadedWrapper =
catalogManager.loadCatalogAndWrap(ident);
+
+ Assertions.assertSame(freshWrapper, loadedWrapper);
+ Assertions.assertSame(freshWrapper,
catalogManager.getCatalogCache().getIfPresent(ident));
+ Mockito.verify(freshWrapper, Mockito.never()).close();
+ } finally {
+ catalogManager.getCatalogCache().invalidate(ident);
+ }
+ }
+
@Test
void testAlterMutableProperties() {
NameIdentifier ident = NameIdentifier.of("metalake", "test51");
