yuqi1129 commented on code in PR #5067:
URL: https://github.com/apache/gravitino/pull/5067#discussion_r1803197472
##########
api/src/main/java/org/apache/gravitino/Catalog.java:
##########
@@ -98,6 +98,9 @@ enum CloudName {
*/
String PROPERTY_PACKAGE = "package";
+ /** The property indicating the catalog is in use. */
Review Comment:
indicating -> indicates
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -474,6 +488,65 @@ public void testConnection(
}
}
+ @Override
+ public void activateCatalog(NameIdentifier ident)
+ throws NoSuchCatalogException, NonInUseEntityException {
+ try {
+ boolean inUse = catalogInUse(store, ident);
+ if (!inUse) {
+ 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, "true");
+ newCatalogBuilder.withProperties(newProps);
+
+ return newCatalogBuilder.build();
+ });
+ }
Review Comment:
```suggestion
if (inUse) {
return;
}
// Other code
```
You can use this to reduce nesting.
##########
gradle.properties:
##########
@@ -40,4 +40,4 @@ defaultScalaVersion = 2.12
pythonVersion = 3.8
# skipDockerTests is used to skip the tests that require Docker to be running.
-skipDockerTests = true
+skipDockerTests = false
Review Comment:
revert it.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -562,38 +627,44 @@ public Catalog alterCatalog(NameIdentifier ident,
CatalogChange... changes)
}
}
- /**
- * Drops (deletes) the catalog with the specified identifier.
- *
- * @param ident The identifier of the catalog to drop.
- * @return {@code true} if the catalog was successfully dropped, {@code
false} otherwise.
- */
@Override
- public boolean dropCatalog(NameIdentifier ident) {
- // There could be a race issue that someone is using the catalog while we
are dropping it.
- catalogCache.invalidate(ident);
-
+ public boolean dropCatalog(NameIdentifier ident, boolean force)
+ throws NonEmptyEntityException, EntityInUseException {
try {
+ boolean catalogInUse = catalogInUse(store, ident);
+ if (catalogInUse && !force) {
+ throw new EntityInUseException(
+ "Catalog %s is in use, please deactivate it first or use force
option", ident);
+ }
+
+ List<SchemaEntity> schemas =
+ store.list(
+ Namespace.of(ident.namespace().level(0), ident.name()),
+ SchemaEntity.class,
+ EntityType.SCHEMA);
CatalogEntity catalogEntity = store.get(ident, EntityType.CATALOG,
CatalogEntity.class);
- if (catalogEntity.getProvider().equals("kafka")) {
- // Kafka catalog needs to cascade drop the default schema
- List<SchemaEntity> schemas =
- store.list(
- Namespace.of(ident.namespace().level(0), ident.name()),
- SchemaEntity.class,
- EntityType.SCHEMA);
- // If there is only one schema, it must be the default schema, because
we don't allow to
- // drop the default schema.
- if (schemas.size() == 1) {
- return store.delete(ident, EntityType.CATALOG, true);
- }
+
+ if (!schemas.isEmpty()
+ && !force
+ && (!catalogEntity.getProvider().equals("kafka") || schemas.size() >
1)) {
Review Comment:
Please optimize the condition, if the execution goes to
`!catalogEntity.getProvider().equals("kafka") || schemas.size() > 1`,
`schemas` must be NOT empty.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java:
##########
@@ -187,6 +187,70 @@ public Response testConnection(
}
}
+ @GET
+ @Path("{catalog}/activate")
+ @Produces("application/vnd.gravitino.v1+json")
+ @Timed(name = "activate-catalog." + MetricNames.HTTP_PROCESS_DURATION,
absolute = true)
+ @ResponseMetered(name = "activate-catalog", absolute = true)
+ public Response activateCatalog(
+ @PathParam("metalake") String metalake, @PathParam("catalog") String
catalogName) {
+ LOG.info("Received activate request for catalog: {}.{}", metalake,
catalogName);
+ try {
+ return Utils.doAs(
+ httpRequest,
+ () -> {
+ NameIdentifier ident = NameIdentifierUtil.ofCatalog(metalake,
catalogName);
+ TreeLockUtils.doWithTreeLock(
+ NameIdentifierUtil.ofMetalake(metalake),
+ LockType.READ,
Review Comment:
Why do you use `READ` here? Does it mean that multiple threads can do the
activated work simultaneously?
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -691,6 +807,23 @@ private CatalogWrapper createCatalogWrapper(CatalogEntity
entity) {
return wrapper;
}
+ /**
+ * get the resolved properties (filter out the hidden properties and add
some required default
Review Comment:
Get
##########
api/src/main/java/org/apache/gravitino/Catalog.java:
##########
@@ -98,6 +98,9 @@ enum CloudName {
*/
String PROPERTY_PACKAGE = "package";
+ /** The property indicating the catalog is in use. */
Review Comment:
indicating -> indicates
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -562,38 +627,44 @@ public Catalog alterCatalog(NameIdentifier ident,
CatalogChange... changes)
}
}
- /**
- * Drops (deletes) the catalog with the specified identifier.
- *
- * @param ident The identifier of the catalog to drop.
- * @return {@code true} if the catalog was successfully dropped, {@code
false} otherwise.
- */
@Override
- public boolean dropCatalog(NameIdentifier ident) {
- // There could be a race issue that someone is using the catalog while we
are dropping it.
- catalogCache.invalidate(ident);
-
+ public boolean dropCatalog(NameIdentifier ident, boolean force)
+ throws NonEmptyEntityException, EntityInUseException {
try {
+ boolean catalogInUse = catalogInUse(store, ident);
+ if (catalogInUse && !force) {
+ throw new EntityInUseException(
+ "Catalog %s is in use, please deactivate it first or use force
option", ident);
+ }
+
+ List<SchemaEntity> schemas =
+ store.list(
+ Namespace.of(ident.namespace().level(0), ident.name()),
+ SchemaEntity.class,
+ EntityType.SCHEMA);
CatalogEntity catalogEntity = store.get(ident, EntityType.CATALOG,
CatalogEntity.class);
- if (catalogEntity.getProvider().equals("kafka")) {
- // Kafka catalog needs to cascade drop the default schema
- List<SchemaEntity> schemas =
- store.list(
- Namespace.of(ident.namespace().level(0), ident.name()),
- SchemaEntity.class,
- EntityType.SCHEMA);
- // If there is only one schema, it must be the default schema, because
we don't allow to
- // drop the default schema.
- if (schemas.size() == 1) {
- return store.delete(ident, EntityType.CATALOG, true);
- }
+
+ if (!schemas.isEmpty()
+ && !force
+ && (!catalogEntity.getProvider().equals("kafka") || schemas.size() >
1)) {
Review Comment:
Please optimize the condition, if the execution goes to
`!catalogEntity.getProvider().equals("kafka") || schemas.size() > 1`,
`schemas` must be NOT empty.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java:
##########
@@ -187,6 +187,70 @@ public Response testConnection(
}
}
+ @GET
+ @Path("{catalog}/activate")
+ @Produces("application/vnd.gravitino.v1+json")
+ @Timed(name = "activate-catalog." + MetricNames.HTTP_PROCESS_DURATION,
absolute = true)
+ @ResponseMetered(name = "activate-catalog", absolute = true)
+ public Response activateCatalog(
+ @PathParam("metalake") String metalake, @PathParam("catalog") String
catalogName) {
+ LOG.info("Received activate request for catalog: {}.{}", metalake,
catalogName);
+ try {
+ return Utils.doAs(
+ httpRequest,
+ () -> {
+ NameIdentifier ident = NameIdentifierUtil.ofCatalog(metalake,
catalogName);
+ TreeLockUtils.doWithTreeLock(
+ NameIdentifierUtil.ofMetalake(metalake),
+ LockType.READ,
Review Comment:
Why do you use `READ` here? Does it mean that multiple threads can do the
activated work simultaneously?
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -474,6 +488,65 @@ public void testConnection(
}
}
+ @Override
+ public void activateCatalog(NameIdentifier ident)
+ throws NoSuchCatalogException, NonInUseEntityException {
+ try {
+ boolean inUse = catalogInUse(store, ident);
+ if (!inUse) {
+ 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, "true");
+ newCatalogBuilder.withProperties(newProps);
+
+ return newCatalogBuilder.build();
+ });
+ }
Review Comment:
```suggestion
if (inUse) {
return;
}
// Other code
```
You can use this to reduce nesting.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -691,6 +807,23 @@ private CatalogWrapper createCatalogWrapper(CatalogEntity
entity) {
return wrapper;
}
+ /**
+ * get the resolved properties (filter out the hidden properties and add
some required default
Review Comment:
Get
##########
gradle.properties:
##########
@@ -40,4 +40,4 @@ defaultScalaVersion = 2.12
pythonVersion = 3.8
# skipDockerTests is used to skip the tests that require Docker to be running.
-skipDockerTests = true
+skipDockerTests = false
Review Comment:
revert it.
--
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]
