[#5066] refactor(drop-catalog): re-define drop catalog #5067

mchades · 2024-10-08T13:21:27Z

What changes were proposed in this pull request?

Add an in-use property to the catalog with the default value of true.
Only empty catalog with in-use=false can be dropped.
User can use dorce option to drop catalog
More drop catalog limitations please see the JavaDoc of dropCatalog in API module

Why are the changes needed?

Fix: #5066

Does this PR introduce any user-facing change?

yes, users now can not drop an in used catalog

How was this patch tested?

tests added

jerryshao

Do you also need to update the doc?

api/src/main/java/org/apache/gravitino/exceptions/NonInUseEntityException.java

clients/client-java/src/main/java/org/apache/gravitino/client/ErrorHandlers.java

jerryshao · 2024-10-16T14:15:24Z

Just did a cursory review, I'm fine with the current implementation. I will spend more time to have a deep review.

api/src/main/java/org/apache/gravitino/Catalog.java

core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java

server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java

gradle.properties

core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java

yuqi1129 · 2024-10-16T14:41:06Z

core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java

+
+      if (!schemas.isEmpty()
+          && !force
+          && (!catalogEntity.getProvider().equals("kafka") || schemas.size() > 1)) {


Please optimize the condition, if the execution goes to !catalogEntity.getProvider().equals("kafka") || schemas.size() > 1, schemas must be NOT empty.

no. schemas.size() may equals 1

The expression here is difficult to understand, maybe we can use multiple if to describe the exception message for different cases.

jerryshao · 2024-10-17T01:58:29Z

api/src/main/java/org/apache/gravitino/exceptions/EntityInUseException.java

+import com.google.errorprone.annotations.FormatString;
+
+/** Exception thrown when an entity is in use and cannot be deleted. */
+public class EntityInUseException extends GravitinoRuntimeException {


Is it better to define a CatalogInUseException and CatalogNotInUseException clearly? This also applies to metalake.

The error message will describe the reason exactly (what entity is not in use).

If we define CatalogInUseException, CatalogNotInUseException, MetalakeInUseException and MetalakeInUseException, then we also need SchemaNotInUseException, TableNotInUseException, TopicNotInUseException, etc which will make the code more complicated, so I think current exception define is enough.

jerqi · 2024-10-17T02:44:37Z

server/src/main/java/org/apache/gravitino/server/web/Utils.java

+  }
+
+  public static Response inUse(String type, String message, Throwable throwable) {
+    return Response.status(Response.Status.CONFLICT)


Maybe I prefer 503. Just a discussion not a suggestion.

SERVICE_UNAVAILABLE(503, "Service Unavailable") means the server status, not the resource. So I think it's not suitable.

jerryshao · 2024-10-17T06:24:41Z

clients/client-python/gravitino/client/gravitino_metalake.py

        """
+        params = {"force": str(force)}


str(force) will get "True/False" or "true/false"?

In any case, the server side will ignore case sensitivity during query param conversion. So it's not a problem

common/src/main/java/org/apache/gravitino/dto/responses/ErrorResponse.java

mchades · 2024-10-17T07:05:30Z

Do you also need to update the doc?

Doc will be updated in final PR

jerryshao · 2024-10-17T07:17:45Z

core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java

+      CatalogEntity catalogEntity =
+          store.get(catalogIdent, EntityType.CATALOG, CatalogEntity.class);
+      return (boolean)
+          BASIC_CATALOG_PROPERTIES_METADATA.getOrDefault(


Why do we need a BASIC_CATALOG_PROPERTIES_METADATA?

jerryshao · 2024-10-17T07:22:48Z

server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java

@@ -187,6 +187,70 @@ public Response testConnection(
    }
  }

+  @GET


Is it better to use PUT or POST?

jerryshao · 2024-10-17T07:34:04Z

core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java

+        protected Map<String, PropertyEntry<?>> specificPropertyEntries() {
+          return Collections.emptyMap();
+        }
+      };


We can create a singleton in BaseCatalogPropertiesMetadata and remove abstract in this class to make it simple.

mchades · 2024-10-17T12:40:30Z

All comments are resolved, plz help to review again, thx! @yuqi1129 @jerryshao

jerryshao · 2024-10-17T14:10:36Z

Can you please fix the CI failure?

jerryshao · 2024-10-17T14:12:32Z

api/src/main/java/org/apache/gravitino/SupportsCatalogs.java

+      throws NonEmptyEntityException, CatalogInUseException;
+
+  /**
+   * Enable a catalog. If the catalog is already enable, this method does nothing.


"If the catalog is already enabled..."

jerryshao · 2024-10-17T14:26:24Z

core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java

+      if (catalogInUse) {
+        // force is true, disable the catalog first
+        disableCatalog(ident);
+      }


Since the catalog will be deleted, I don't think it is necessary to do the disable here.

The purpose of this operation is to modify the in-use value in the backend store

jerryshao · 2024-10-17T14:28:07Z

core/src/main/java/org/apache/gravitino/listener/CatalogEventDispatcher.java

+  @Override
+  public void enableCatalog(NameIdentifier ident)
+      throws NoSuchCatalogException, CatalogNotInUseException {
+    // todo: support activate catalog event


Will you do this in a different PR?

…5067) ### What changes were proposed in this pull request? - Add an `in-use` property to the catalog with the default value of true. - Only empty catalog with `in-use=false` can be dropped. - User can use `dorce` option to drop catalog - More drop catalog limitations please see the JavaDoc of `dropCatalog` in API module ### Why are the changes needed? Fix: apache#5066 ### Does this PR introduce _any_ user-facing change? yes, users now can not drop an in used catalog ### How was this patch tested? tests added

mchades self-assigned this Oct 8, 2024

mchades force-pushed the drop-catalog branch 8 times, most recently from fe68851 to cb738a2 Compare October 14, 2024 10:12

mchades force-pushed the drop-catalog branch from cb738a2 to 17b7967 Compare October 16, 2024 07:46

mchades mentioned this pull request Oct 16, 2024

[#5154] refactor(drop-metalake): re-define drop metalake #5155

Merged

mchades requested review from jerryshao and jerqi October 16, 2024 07:55

mchades force-pushed the drop-catalog branch 2 times, most recently from 55b0e72 to 5f5e913 Compare October 16, 2024 11:19

jerryshao reviewed Oct 16, 2024

View reviewed changes

yuqi1129 reviewed Oct 16, 2024

View reviewed changes

jerryshao reviewed Oct 17, 2024

View reviewed changes

jerqi reviewed Oct 17, 2024

View reviewed changes

jerryshao reviewed Oct 17, 2024

View reviewed changes

common/src/main/java/org/apache/gravitino/dto/responses/ErrorResponse.java Outdated Show resolved Hide resolved

mchades added 2 commits October 17, 2024 14:58

re-define drop catalog

770294d

address comments

2a910f8

mchades force-pushed the drop-catalog branch from d1af461 to 2a910f8 Compare October 17, 2024 06:59

change read lock to write

c2f7a6e

jerryshao reviewed Oct 17, 2024

View reviewed changes

mchades added 2 commits October 17, 2024 16:34

Align exception names

f5b34b4

rename active to enable and rename deactivate to disable

33d7171

mchades added 3 commits October 17, 2024 18:30

make BASIC_CATALOG_PROPERTIES_METADATA singleton

7fefe99

use PATCH method for REST API

fab7bef

refactor if statement

7429161

fix CI

2938fd1

mchades force-pushed the drop-catalog branch from f4e137d to 2938fd1 Compare October 17, 2024 14:23

jerryshao reviewed Oct 17, 2024

View reviewed changes

mchades added 2 commits October 18, 2024 09:53

fix comment

45f658d

remove disable in drop

c0aad94

jerryshao approved these changes Oct 18, 2024

View reviewed changes

jerryshao merged commit 639dfc8 into apache:main Oct 18, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#5066] refactor(drop-catalog): re-define drop catalog #5067

[#5066] refactor(drop-catalog): re-define drop catalog #5067

mchades commented Oct 8, 2024

jerryshao left a comment

jerryshao commented Oct 16, 2024

yuqi1129 Oct 16, 2024

mchades Oct 17, 2024

jerryshao Oct 17, 2024

mchades Oct 17, 2024

jerryshao Oct 17, 2024

mchades Oct 17, 2024

jerqi Oct 17, 2024

mchades Oct 17, 2024

jerryshao Oct 17, 2024

mchades Oct 17, 2024

mchades commented Oct 17, 2024

jerryshao Oct 17, 2024

jerryshao Oct 17, 2024

jerryshao Oct 17, 2024

mchades commented Oct 17, 2024

jerryshao commented Oct 17, 2024

jerryshao Oct 17, 2024

jerryshao Oct 17, 2024

mchades Oct 17, 2024

mchades Oct 18, 2024

jerryshao Oct 17, 2024

mchades Oct 17, 2024

@@ @@ -187,6 +187,70 @@ public Response testConnection( @@
                   }
                 }
+                @GET

[#5066] refactor(drop-catalog): re-define drop catalog #5067

[#5066] refactor(drop-catalog): re-define drop catalog #5067

Conversation

mchades commented Oct 8, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

jerryshao left a comment

Choose a reason for hiding this comment

jerryshao commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchades commented Oct 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchades commented Oct 17, 2024

jerryshao commented Oct 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment