This is an automated email from the ASF dual-hosted git repository.
yuqi4733 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 bdb9939785 [#7251] Make `catalogCache` in `CatalogManager` as a
instance field not a static value. (#7297)
bdb9939785 is described below
commit bdb99397852c9f968784a109460e041ebb445feb
Author: Gagan B Mishra <[email protected]>
AuthorDate: Mon Jun 2 22:44:31 2025 -0700
[#7251] Make `catalogCache` in `CatalogManager` as a instance field not a
static value. (#7297)
### What changes were proposed in this pull request?
Changed the catalogCache to be an instance variable instead of static.
This was done to avoid creating the static var from within the
constructor.
This cleaned up some unit tests and places where constructor was first
called to initialize the cache, and then the cache was accessed in a
static manner.
### Why are the changes needed?
Code improvements.
Fix: #7251
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
./gradlew clean build
---
.../org/apache/gravitino/catalog/CatalogManager.java | 12 ++++++------
.../apache/gravitino/catalog/OperationDispatcher.java | 5 ++---
.../apache/gravitino/catalog/TestCatalogManager.java | 18 +++++++++---------
3 files changed, 17 insertions(+), 18 deletions(-)
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 00397ad254..b859d25849 100644
--- a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
+++ b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
@@ -30,7 +30,6 @@ import static
org.apache.gravitino.metalake.MetalakeManager.metalakeInUse;
import com.github.benmanes.caffeine.cache.Cache;
import com.github.benmanes.caffeine.cache.Caffeine;
import com.github.benmanes.caffeine.cache.Scheduler;
-import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
@@ -57,6 +56,7 @@ import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import javax.annotation.Nullable;
+import lombok.Getter;
import org.apache.commons.io.FileUtils;
import org.apache.commons.lang3.tuple.Pair;
import org.apache.gravitino.Catalog;
@@ -115,7 +115,7 @@ public class CatalogManager implements CatalogDispatcher,
Closeable {
private static final Logger LOG =
LoggerFactory.getLogger(CatalogManager.class);
- public static void checkCatalogInUse(EntityStore store, NameIdentifier ident)
+ public void checkCatalogInUse(EntityStore store, NameIdentifier ident)
throws NoSuchMetalakeException, NoSuchCatalogException,
CatalogNotInUseException,
MetalakeNotInUseException {
NameIdentifier metalakeIdent =
NameIdentifier.of(ident.namespace().levels());
@@ -262,7 +262,7 @@ public class CatalogManager implements CatalogDispatcher,
Closeable {
private final Config config;
- @VisibleForTesting static Cache<NameIdentifier, CatalogWrapper> catalogCache;
+ @Getter private final Cache<NameIdentifier, CatalogWrapper> catalogCache;
private final EntityStore store;
@@ -281,7 +281,7 @@ public class CatalogManager implements CatalogDispatcher,
Closeable {
this.idGenerator = idGenerator;
long cacheEvictionIntervalInMs =
config.get(Configs.CATALOG_CACHE_EVICTION_INTERVAL_MS);
- catalogCache =
+ this.catalogCache =
Caffeine.newBuilder()
.expireAfterAccess(cacheEvictionIntervalInMs,
TimeUnit.MILLISECONDS)
.removalListener(
@@ -840,13 +840,13 @@ public class CatalogManager implements CatalogDispatcher,
Closeable {
return catalogCache.get(ident, this::loadCatalogInternal);
}
- private static boolean catalogInUse(EntityStore store, NameIdentifier ident)
+ private boolean catalogInUse(EntityStore store, NameIdentifier ident)
throws NoSuchMetalakeException, NoSuchCatalogException {
NameIdentifier metalakeIdent =
NameIdentifier.of(ident.namespace().levels());
return metalakeInUse(store, metalakeIdent) && getCatalogInUseValue(store,
ident);
}
- private static boolean getCatalogInUseValue(EntityStore store,
NameIdentifier catalogIdent) {
+ private boolean getCatalogInUseValue(EntityStore store, NameIdentifier
catalogIdent) {
try {
CatalogWrapper wrapper = catalogCache.getIfPresent(catalogIdent);
CatalogEntity catalogEntity;
diff --git
a/core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java
b/core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java
index 91d2d66caa..a9e4d73ef1 100644
--- a/core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java
+++ b/core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java
@@ -18,7 +18,6 @@
*/
package org.apache.gravitino.catalog;
-import static org.apache.gravitino.catalog.CatalogManager.checkCatalogInUse;
import static
org.apache.gravitino.catalog.PropertiesMetadataHelpers.validatePropertyForAlter;
import static
org.apache.gravitino.utils.NameIdentifierUtil.getCatalogIdentifier;
@@ -93,7 +92,7 @@ public abstract class OperationDispatcher {
protected <R, E extends Throwable> R doWithCatalog(
NameIdentifier ident, ThrowableFunction<CatalogManager.CatalogWrapper,
R> fn, Class<E> ex)
throws E {
- checkCatalogInUse(store, ident);
+ catalogManager.checkCatalogInUse(store, ident);
try {
CatalogManager.CatalogWrapper c =
catalogManager.loadCatalogAndWrap(ident);
@@ -115,7 +114,7 @@ public abstract class OperationDispatcher {
Class<E1> ex1,
Class<E2> ex2)
throws E1, E2 {
- checkCatalogInUse(store, ident);
+ catalogManager.checkCatalogInUse(store, ident);
try {
CatalogManager.CatalogWrapper c =
catalogManager.loadCatalogAndWrap(ident);
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 acb8886a52..b9cede7473 100644
--- a/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java
+++ b/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java
@@ -300,7 +300,7 @@ public class TestCatalogManager {
testProperties(props, testCatalog.properties());
Assertions.assertEquals(Catalog.Type.RELATIONAL, testCatalog.type());
- Assertions.assertNotNull(CatalogManager.catalogCache.getIfPresent(ident));
+
Assertions.assertNotNull(catalogManager.getCatalogCache().getIfPresent(ident));
// test before creation
NameIdentifier ident2 = NameIdentifier.of("metalake1", "test1");
@@ -318,7 +318,7 @@ public class TestCatalogManager {
catalogManager.createCatalog(
ident2, Catalog.Type.RELATIONAL, provider, "comment",
props));
Assertions.assertTrue(exception1.getMessage().contains("Metalake metalake1
does not exist"));
- Assertions.assertNull(CatalogManager.catalogCache.getIfPresent(ident2));
+
Assertions.assertNull(catalogManager.getCatalogCache().getIfPresent(ident2));
// test before creation
Assertions.assertThrows(
@@ -338,7 +338,7 @@ public class TestCatalogManager {
exception2.getMessage().contains("Catalog metalake.test1 already
exists"));
// Test if the catalog is already cached
- CatalogManager.CatalogWrapper cached =
CatalogManager.catalogCache.getIfPresent(ident);
+ CatalogManager.CatalogWrapper cached =
catalogManager.getCatalogCache().getIfPresent(ident);
Assertions.assertNotNull(cached);
// Test failed creation
@@ -355,7 +355,7 @@ public class TestCatalogManager {
.getMessage()
.contains("Properties or property prefixes are reserved and cannot
be set"),
exception3.getMessage());
-
Assertions.assertNull(CatalogManager.catalogCache.getIfPresent(failedIdent));
+
Assertions.assertNull(catalogManager.getCatalogCache().getIfPresent(failedIdent));
// Test failed for the second time
Throwable exception4 =
Assertions.assertThrows(
@@ -368,7 +368,7 @@ public class TestCatalogManager {
.getMessage()
.contains("Properties or property prefixes are reserved and cannot
be set"),
exception4.getMessage());
-
Assertions.assertNull(CatalogManager.catalogCache.getIfPresent(failedIdent));
+
Assertions.assertNull(catalogManager.getCatalogCache().getIfPresent(failedIdent));
}
@Test
@@ -475,7 +475,7 @@ public class TestCatalogManager {
exception.getMessage().contains("Catalog metalake.test22 does not
exist"));
// Load operation will cache the catalog
- Assertions.assertNotNull(CatalogManager.catalogCache.getIfPresent(ident));
+
Assertions.assertNotNull(catalogManager.getCatalogCache().getIfPresent(ident));
}
@Test
@@ -529,8 +529,8 @@ public class TestCatalogManager {
exception.getMessage().contains("Catalog metalake.test33 does not
exist"));
// Alter operation will update the cache
- Assertions.assertNull(CatalogManager.catalogCache.getIfPresent(ident));
- Assertions.assertNotNull(CatalogManager.catalogCache.getIfPresent(ident1));
+
Assertions.assertNull(catalogManager.getCatalogCache().getIfPresent(ident));
+
Assertions.assertNotNull(catalogManager.getCatalogCache().getIfPresent(ident1));
}
@Test
@@ -566,7 +566,7 @@ public class TestCatalogManager {
Assertions.assertFalse(dropped1);
// Drop operation will update the cache
- Assertions.assertNull(CatalogManager.catalogCache.getIfPresent(ident));
+
Assertions.assertNull(catalogManager.getCatalogCache().getIfPresent(ident));
}
@Test
