-
Notifications
You must be signed in to change notification settings - Fork 714
[#9586] improvement(core): optimize checking in use status of catalogs and metalakes #9574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR optimizes the "in-use" checking mechanism for metalakes and catalogs by introducing a new property tracking system. The changes move catalog validation logic from the dispatcher layer to the catalog initialization layer and add synchronization between metalake and catalog in-use states.
Key Changes
- Introduces
PROPERTY_METALAKE_IN_USEconstant to track whether the metalake containing a catalog is enabled - Modifies
enableMetalakeanddisableMetalaketo update the new property on all child catalogs - Moves catalog in-use validation from
OperationDispatcherInterceptortoBaseCatalog.ops()initialization
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| api/src/main/java/org/apache/gravitino/Catalog.java | Adds new PROPERTY_METALAKE_IN_USE constant |
| core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java | Updates enable/disable metalake methods to propagate in-use state to catalogs |
| core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java | Adds updateCatalogProperty method and removes checkCatalogInUse method |
| core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java | Adds validation logic to check metalake and catalog in-use status during ops initialization |
| core/src/main/java/org/apache/gravitino/connector/BaseCatalogPropertiesMetadata.java | Attempts to register new property entry (contains critical bug) |
| core/src/main/java/org/apache/gravitino/catalog/OperationDispatcherInterceptor.java | Removes catalog in-use check and adds unused variable suppressions |
core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/catalog/OperationDispatcherInterceptor.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/connector/BaseCatalogPropertiesMetadata.java
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
Outdated
Show resolved
Hide resolved
|
@jerryshao
This draft PR is the skeleton of the changes mentioned above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.
Comments suppressed due to low confidence (1)
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:648
- The removal of the metalake check and the early-return logic in disableCatalog changes the method's behavior. Previously, the method would check if the metalake exists and if the catalog is already disabled before proceeding. Now it always attempts to update the catalog. This could result in unnecessary entity store operations and cache invalidations when the catalog is already disabled. Consider restoring the early-return optimization to avoid unnecessary work.
metalakeIdent,
LockType.WRITE,
() -> {
try {
store.update(
ident,
CatalogEntity.class,
EntityType.CATALOG,
catalog -> {
CatalogEntity.Builder newCatalogBuilder =
newCatalogBuilder(ident.namespace(), catalog);
Map<String, String> newProps =
catalog.getProperties() == null
? new HashMap<>()
: new HashMap<>(catalog.getProperties());
newProps.put(PROPERTY_IN_USE, "false");
newCatalogBuilder.withProperties(newProps);
return newCatalogBuilder.build();
});
catalogCache.invalidate(ident);
return null;
} catch (IOException e) {
throw new RuntimeException(e);
}
});
}
/**
core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
Outdated
Show resolved
Hide resolved
|
Combine with #9430 , the result are quite obvious compared to that in the main branch.
|
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
Outdated
Show resolved
Hide resolved
|
@mchades |
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
Outdated
Show resolved
Hide resolved
|
@mchades |
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
Outdated
Show resolved
Hide resolved
mchades
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
current changes are LGTM.
@jerryshao could you plz also take a review? thx
|
You can go ahead if you make sure the logic is correct. |

What changes were proposed in this pull request?
This pull request refactors catalog usage validation in the Gravitino codebase, improving how the system checks whether catalogs and their parent metalakes are enabled ("in use"). The main changes include removing the dynamic proxy-based
OperationDispatcherInterceptor, centralizing usage checks within the catalog implementation, and introducing a new property to track metalake usage status. Additionally, the code now ensures that operations on catalogs consistently verify both catalog and metalake usage status.Why are the changes needed?
To clean and simply code path
Fix: #9586
Does this PR introduce any user-facing change?
N/A.
How was this patch tested?
UTs