Copilot commented on code in PR #9574:
URL: https://github.com/apache/gravitino/pull/9574#discussion_r2652420432
##########
core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java:
##########
@@ -175,6 +176,8 @@ public CatalogOperations ops() {
Preconditions.checkArgument(
entity != null && conf != null, "entity and conf must be set
before calling ops()");
CatalogOperations newOps = createOps(conf);
+ // Check metalake and catalogInuse
Review Comment:
The comment on line 179 says "Check metalake and catalogInuse" but should
say "Check metalake and catalog in use" (with a space). This is a minor
grammar/formatting issue.
```suggestion
// Check metalake and catalog in use
```
##########
core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java:
##########
@@ -175,6 +176,8 @@ public CatalogOperations ops() {
Preconditions.checkArgument(
entity != null && conf != null, "entity and conf must be set
before calling ops()");
CatalogOperations newOps = createOps(conf);
+ // Check metalake and catalogInuse
+ checkMetalakeAndCatalogInUse(entity);
Review Comment:
The check for metalake and catalog in-use status happens lazily when ops()
is called, rather than eagerly when the catalog is loaded. This means a catalog
that was valid when loaded can fail later when ops() is accessed. This could
lead to confusing behavior where catalog.ops() throws an exception even though
the catalog was successfully loaded. Consider documenting this lazy-check
behavior or moving the check to an earlier point in the catalog lifecycle.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -591,6 +573,44 @@ public void enableCatalog(NameIdentifier ident)
});
}
+ public void updateCatalogProperty(
+ NameIdentifier nameIdentifier, String propertyKey, String propertyValue)
{
Review Comment:
The updateCatalogProperty method is public and can be called by any code,
but it appears to be designed for internal use by MetalakeManager. This breaks
encapsulation and could allow external code to bypass validation logic.
Consider making this method package-private or adding appropriate access
controls and validation.
```suggestion
NameIdentifier nameIdentifier, String propertyKey, String
propertyValue) {
// Validate property key and value against catalog properties metadata
to avoid bypassing
// validation performed in standard catalog creation/alter flows.
validatePropertyForAlter(BASIC_CATALOG_PROPERTIES_METADATA, propertyKey,
propertyValue);
```
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -646,8 +661,6 @@ public Catalog alterCatalog(NameIdentifier ident,
CatalogChange... changes)
ident,
LockType.READ,
() -> {
Review Comment:
The removal of the checkCatalogInUse method and the removal of catalog
in-use validation from alterCatalog could be a breaking change. Previously,
alterCatalog would check if the catalog was in use before allowing
modifications. Now this check is removed, which means catalogs can be altered
even when disabled. This might be intentional, but it represents a significant
behavior change that should be documented in the PR description and potentially
in user-facing documentation.
##########
core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java:
##########
@@ -175,6 +176,8 @@ public CatalogOperations ops() {
Preconditions.checkArgument(
entity != null && conf != null, "entity and conf must be set
before calling ops()");
CatalogOperations newOps = createOps(conf);
+ // Check metalake and catalogInuse
+ checkMetalakeAndCatalogInUse(entity);
Review Comment:
The checkMetalakeAndCatalogInUse method is called every time ops() is
accessed, which could be frequently in a high-traffic system. This adds
overhead to every catalog operation. The previous
OperationDispatcherInterceptor approach had similar overhead but at least
documented this trade-off. Consider caching the in-use status or documenting
the performance implications of this design choice.
##########
core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java:
##########
@@ -376,24 +396,42 @@ public void disableMetalake(NameIdentifier ident) throws
NoSuchMetalakeException
() -> {
try {
boolean inUse = metalakeInUse(store, ident);
- if (inUse) {
- store.update(
- ident,
- BaseMetalake.class,
- EntityType.METALAKE,
- metalake -> {
- BaseMetalake.Builder builder =
newMetalakeBuilder(metalake);
-
- Map<String, String> newProps =
- metalake.properties() == null
- ? Maps.newHashMap()
- : Maps.newHashMap(metalake.properties());
- newProps.put(PROPERTY_IN_USE, "false");
- builder.withProperties(newProps);
-
- return builder.build();
- });
+ if (!inUse) {
+ return null;
}
+
+ store.update(
+ ident,
+ BaseMetalake.class,
+ EntityType.METALAKE,
+ metalake -> {
+ BaseMetalake.Builder builder = newMetalakeBuilder(metalake);
+
+ Map<String, String> newProps =
+ metalake.properties() == null
+ ? Maps.newHashMap()
+ : Maps.newHashMap(metalake.properties());
+ newProps.put(PROPERTY_IN_USE, "false");
+ builder.withProperties(newProps);
+
+ return builder.build();
+ });
+
+ // The only problem is that we can't make sure we can change all
catalog properties
+ // in a transaction, if any of them fails, the metalake is already
enabled and the value
+ // in catalog is inconsistent.
+ store
+ .list(Namespace.of(ident.name()), CatalogEntity.class,
EntityType.CATALOG)
+ .forEach(
+ catalogEntity -> {
+ // update the properties metalake-in-use in catalog to
false
+ GravitinoEnv.getInstance()
+ .catalogManager()
+ .updateCatalogProperty(
+ catalogEntity.nameIdentifier(),
+ Catalog.PROPERTY_METALAKE_IN_USE,
+ "false");
+ });
Review Comment:
The same transaction safety issue exists here as in enableMetalake. If any
catalog property update fails after the metalake has been disabled, the system
will be in an inconsistent state. Additionally, the comment on line 421
incorrectly says "the metalake is already enabled" when it should say "the
metalake is already disabled".
##########
api/src/main/java/org/apache/gravitino/Catalog.java:
##########
@@ -132,6 +132,11 @@ enum CloudName {
/** The property indicates the catalog is in use. */
String PROPERTY_IN_USE = "in-use";
+ /**
+ * The property indicates the Metalake catalog is in use and this catalog is
managed by Metalake.
Review Comment:
The documentation for PROPERTY_METALAKE_IN_USE is unclear and potentially
misleading. The phrase "Metalake catalog is in use and this catalog is managed
by Metalake" is confusing - it's unclear if this means the metalake is in use,
or if this is describing a "Metalake catalog" (which isn't a standard term).
The documentation should clearly state: "This property indicates whether the
metalake that contains this catalog is in use. When a metalake is disabled,
this property is set to false on all its catalogs."
```suggestion
* This property indicates whether the metalake that contains this catalog
is in use.
* When a metalake is disabled, this property is set to {@code false} on
all its catalogs.
```
##########
core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java:
##########
@@ -342,25 +344,43 @@ public void enableMetalake(NameIdentifier ident) throws
NoSuchMetalakeException
() -> {
try {
boolean inUse = metalakeInUse(store, ident);
- if (!inUse) {
- store.update(
- ident,
- BaseMetalake.class,
- EntityType.METALAKE,
- metalake -> {
- BaseMetalake.Builder builder =
newMetalakeBuilder(metalake);
-
- Map<String, String> newProps =
- metalake.properties() == null
- ? Maps.newHashMap()
- : Maps.newHashMap(metalake.properties());
- newProps.put(PROPERTY_IN_USE, "true");
- builder.withProperties(newProps);
-
- return builder.build();
- });
+ if (inUse) {
+ return null;
}
+ store.update(
+ ident,
+ BaseMetalake.class,
+ EntityType.METALAKE,
+ metalake -> {
+ BaseMetalake.Builder builder = newMetalakeBuilder(metalake);
+
+ Map<String, String> newProps =
+ metalake.properties() == null
+ ? Maps.newHashMap()
+ : Maps.newHashMap(metalake.properties());
+ newProps.put(PROPERTY_IN_USE, "true");
+ builder.withProperties(newProps);
+
+ return builder.build();
+ });
+
+ // The only problem is that we can't make sure we can change all
catalog properties
+ // in a transaction. If any catalog property update fails, the
metalake is already
+ // enabled but catalog properties remain inconsistent.
+ store
+ .list(Namespace.of(ident.name()), CatalogEntity.class,
EntityType.CATALOG)
+ .forEach(
+ catalogEntity -> {
+ // update the properties metalake-in-use in catalog to
false
Review Comment:
The comment on line 375 incorrectly states "update the properties
metalake-in-use in catalog to false" when the actual code sets it to "true".
This comment appears to be copied from disableMetalake without being updated
for enableMetalake.
```suggestion
// update the properties metalake-in-use in catalog to
true
```
##########
core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java:
##########
@@ -342,25 +344,43 @@ public void enableMetalake(NameIdentifier ident) throws
NoSuchMetalakeException
() -> {
try {
boolean inUse = metalakeInUse(store, ident);
- if (!inUse) {
- store.update(
- ident,
- BaseMetalake.class,
- EntityType.METALAKE,
- metalake -> {
- BaseMetalake.Builder builder =
newMetalakeBuilder(metalake);
-
- Map<String, String> newProps =
- metalake.properties() == null
- ? Maps.newHashMap()
- : Maps.newHashMap(metalake.properties());
- newProps.put(PROPERTY_IN_USE, "true");
- builder.withProperties(newProps);
-
- return builder.build();
- });
+ if (inUse) {
+ return null;
}
+ store.update(
+ ident,
+ BaseMetalake.class,
+ EntityType.METALAKE,
+ metalake -> {
+ BaseMetalake.Builder builder = newMetalakeBuilder(metalake);
+
+ Map<String, String> newProps =
+ metalake.properties() == null
+ ? Maps.newHashMap()
+ : Maps.newHashMap(metalake.properties());
+ newProps.put(PROPERTY_IN_USE, "true");
+ builder.withProperties(newProps);
+
+ return builder.build();
+ });
+
+ // The only problem is that we can't make sure we can change all
catalog properties
+ // in a transaction. If any catalog property update fails, the
metalake is already
+ // enabled but catalog properties remain inconsistent.
+ store
+ .list(Namespace.of(ident.name()), CatalogEntity.class,
EntityType.CATALOG)
+ .forEach(
+ catalogEntity -> {
+ // update the properties metalake-in-use in catalog to
false
+ GravitinoEnv.getInstance()
+ .catalogManager()
+ .updateCatalogProperty(
+ catalogEntity.nameIdentifier(),
+ Catalog.PROPERTY_METALAKE_IN_USE,
+ "true");
+ });
Review Comment:
The changes to enableMetalake and disableMetalake methods that now update
catalog properties lack unit test coverage. The new behavior of propagating
metalake status to all child catalogs is a significant functional change that
needs test coverage to ensure it works correctly, especially for edge cases
like when catalogs don't exist or when updates fail.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -591,6 +573,44 @@ public void enableCatalog(NameIdentifier ident)
});
}
+ public void updateCatalogProperty(
+ NameIdentifier nameIdentifier, String propertyKey, String propertyValue)
{
+ try {
+ store.update(
+ nameIdentifier,
+ CatalogEntity.class,
+ EntityType.CATALOG,
+ catalog -> {
+ CatalogEntity.Builder newCatalogBuilder =
+ newCatalogBuilder(nameIdentifier.namespace(), catalog);
+
+ Map<String, String> newProps =
+ catalog.getProperties() == null
+ ? new HashMap<>()
+ : new HashMap<>(catalog.getProperties());
+ newProps.put(propertyKey, propertyValue);
+ newCatalogBuilder.withProperties(newProps);
+
+ return newCatalogBuilder.build();
+ });
+ catalogCache.invalidate(nameIdentifier);
+
+ } catch (NoSuchCatalogException e) {
+ LOG.error("Catalog {} does not exist", nameIdentifier, e);
+ throw new RuntimeException(e);
+ } catch (IllegalArgumentException e) {
+ LOG.error(
+ "Failed to update catalog {} property {} with unknown change",
+ nameIdentifier,
+ propertyKey,
+ e);
+ throw e;
+ } catch (IOException ioe) {
+ LOG.error("Failed to update catalog {} property {}", nameIdentifier,
propertyKey, ioe);
+ throw new RuntimeException(ioe);
+ }
+ }
Review Comment:
The new updateCatalogProperty method lacks unit test coverage. This is a
critical method that directly modifies catalog properties in the entity store
and is called during metalake enable/disable operations. Given that the
repository has comprehensive test coverage (TestCatalogManager.java exists with
641 lines), this new functionality should have corresponding unit tests to
verify correct behavior, error handling, and edge cases.
--
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]
