mchades commented on code in PR #9574:
URL: https://github.com/apache/gravitino/pull/9574#discussion_r2655178453
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -592,20 +575,54 @@ public void enableCatalog(NameIdentifier ident)
});
}
+ public void updateCatalogProperty(
Review Comment:
Does this method duplicate `alterCatalog` with `setPropertyChange`?
##########
api/src/main/java/org/apache/gravitino/Catalog.java:
##########
@@ -132,6 +132,12 @@ enum CloudName {
/** The property indicates the catalog is in use. */
String PROPERTY_IN_USE = "in-use";
+ /**
+ * 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.
+ */
+ String PROPERTY_METALAKE_IN_USE = "metalake-in-use";
Review Comment:
Should this property be visible to users?
##########
core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java:
##########
@@ -367,25 +369,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.
Review Comment:
We can try our best to update all catalogs, and log the catalogs that failed
to change in the warning log.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -647,11 +664,9 @@ public Catalog alterCatalog(NameIdentifier ident,
CatalogChange... changes)
ident,
LockType.READ,
() -> {
- checkCatalogInUse(store, ident);
-
// There could be a race issue that someone is using the catalog
from cache while we are
// updating it.
-
+ checkMetalake(NameIdentifier.of(ident.namespace().levels()), store);
Review Comment:
can we check the catalog property instead?
##########
core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java:
##########
@@ -190,6 +194,39 @@ public CatalogOperations ops() {
return ops;
}
+ public void checkMetalakeAndCatalogInUse(CatalogEntity catalogEntity) {
+ Map<String, String> catalogProperties = catalogEntity.getProperties();
+ if (catalogProperties == null) {
+ return;
+ }
+
+ boolean metalakeInuse =
+ Boolean.parseBoolean(
+ catalogProperties.getOrDefault(Catalog.PROPERTY_METALAKE_IN_USE,
"true"));
+ if (!metalakeInuse) {
+ throw new MetalakeNotInUseException(
+ "The metalake that holds catalog %s is not in use",
catalogEntity.name());
+ }
+
+ // Check the stack and find whether its within method `dropCatalog` as we
don't need to check
+ // the catalog in use status when dropping the catalog.
+ if (isInvokedBy("dropCatalog")) {
+ LOG.info("Skip checking catalog in use status since it's invoked by
dropCatalog method.");
+ return;
Review Comment:
when a catalog is in use, we cannot drop it.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -555,18 +543,13 @@ public void testConnection(
public void enableCatalog(NameIdentifier ident)
throws NoSuchCatalogException, CatalogNotInUseException {
NameIdentifier metalakeIdent =
NameIdentifier.of(ident.namespace().levels());
+ checkMetalake(metalakeIdent, store);
Review Comment:
Since we already store the `metalake-in-use` in the catalog, is there a need
to use the `store` to checkMetalake?
--
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]
