yuqi1129 commented on code in PR #9969:
URL: https://github.com/apache/gravitino/pull/9969#discussion_r2793486576
##########
core/src/main/java/org/apache/gravitino/hook/CatalogHookDispatcher.java:
##########
@@ -175,4 +175,9 @@ public void disableCatalog(NameIdentifier ident) throws
NoSuchCatalogException {
public boolean catalogExists(NameIdentifier ident) {
return dispatcher.catalogExists(ident);
}
+
+ @Override
+ public void
addCatalogCacheRemoveListener(java.util.function.Consumer<NameIdentifier>
listener) {
Review Comment:
java.util.function.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergCatalogWrapperManager.java:
##########
@@ -71,6 +72,19 @@ public IcebergCatalogWrapperManager(
.setNameFormat("iceberg-catalog-wrapper-cleaner-%d")
.build())))
.build();
+ IcebergRESTServerContext context = IcebergRESTServerContext.getInstance();
+ if (context.isAuxMode()) {
+ GravitinoEnv.getInstance()
+ .catalogDispatcher()
+ .addCatalogCacheRemoveListener(
+ ident -> {
+ if (ident != null) {
Review Comment:
You have checked the null problem of `ident`.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -304,6 +306,11 @@ public CatalogManager(Config config, EntityStore store,
IdGenerator idGenerator)
.expireAfterAccess(cacheEvictionIntervalInMs,
TimeUnit.MILLISECONDS)
.removalListener(
(k, v, c) -> {
+ for (Consumer<NameIdentifier> listener : removalListeners) {
Review Comment:
`removalListener` will be executed asynchronous and we can make sure when it
was called when an entity was removed. I'm worried there might be a timeliness
issue with this
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogDispatcher.java:
##########
@@ -19,10 +19,26 @@
package org.apache.gravitino.catalog;
+import java.util.function.Consumer;
+import org.apache.gravitino.NameIdentifier;
+
/**
* {@code CatalogDispatcher} interface acts as a specialization of the {@link
SupportsCatalogs}
* interface. This interface is designed to potentially add custom behaviors
or operations related
* to dispatching or handling catalog-related events or actions that are not
covered by the standard
* {@code SupportsCatalogs} operations.
*/
-public interface CatalogDispatcher extends SupportsCatalogs {}
+public interface CatalogDispatcher extends SupportsCatalogs {
+ /**
+ * Adds a listener that will be notified when a catalog is removed from the
cache.
+ *
+ * <p>Note: Cache eviction is invoked asynchronously but uses a single
thread to process removal
+ * events. To avoid blocking the eviction thread and delaying subsequent
cache operations,
+ * listeners should avoid performing heavy operations (such as I/O, network
calls, or complex
+ * computations) directly. Instead, consider offloading heavy work to a
separate thread or
+ * executor.
+ *
+ * @param listener The consumer to be called with the NameIdentifier of the
removed catalog.
+ */
+ void addCatalogCacheRemoveListener(Consumer<NameIdentifier> listener);
Review Comment:
This method is not so general in the interface and is only targeted for the
implementation that use cache that supports the remove-listener mechanism. Is
there a better name or way to get the goals?
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -304,6 +306,11 @@ public CatalogManager(Config config, EntityStore store,
IdGenerator idGenerator)
.expireAfterAccess(cacheEvictionIntervalInMs,
TimeUnit.MILLISECONDS)
.removalListener(
(k, v, c) -> {
+ for (Consumer<NameIdentifier> listener : removalListeners) {
Review Comment:
Another point is that we need to distinguish whether the catalog was removed
by users or by timeout eviction. I believe we only need to handle the former
case.
--
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]
