[#2434] Improvement: if DropXXX success, return true, if not exists return false #3222

charliecheng630 · 2024-04-29T13:51:57Z

What changes were proposed in this pull request?

When dropping the XXX(Metalake/Catalog/Schema/Table/Fileset/Partition/Topic), if the XXX does not exist, then return false.

Why are the changes needed?

a clear drop behavior of return value and exception thrown.

Fix: #2434

Does this PR introduce any user-facing change?

No

How was this patch tested?

ITs and UTs

jerryshao · 2024-05-06T04:14:34Z

@mchades would you please help to review this?

.../catalog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveCatalogOperations.java

...-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/DatabaseOperation.java

...dbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/TableOperation.java

...on/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcDatabaseOperations.java

...ommon/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcTableOperations.java

...g-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalogOperations.java

mchades · 2024-05-06T07:54:07Z

Are the changes in core module the same as #3100 ?

charliecheng630 · 2024-05-07T01:29:19Z

Are the changes in core module the same as #3100 ?

Yes.

charliecheng630 · 2024-05-07T01:29:56Z

Update. Please help to review again when you have time~

mchades

The JavaDoc for methods(dropXxx) like com.datastrato.gravitino.SupportsMetalakes#dropMetalake should be reviewed and revised if needed.

.../catalog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveCatalogOperations.java

...ommon/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcTableOperations.java

...on/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcDatabaseOperations.java

charliecheng630 · 2024-05-09T01:33:49Z

Updated. Please help to review again when you have time, thanks!

charliecheng630 · 2024-05-09T13:08:16Z

@mchades Updated. Please help to review again when you have time, thanks!

mchades · 2024-05-10T02:24:32Z

.../src/main/java/com/datastrato/gravitino/catalog/doris/converter/DorisExceptionConverter.java

@@ -76,6 +81,10 @@ static int getErrorCodeFromMessage(String message) {
      return CODE_DATABASE_EXISTS;
    }

+    if (DATABASE_NOT_EXISTS_PATTERN.matcher(message).matches()) {
+      return CODE_DATABASE_NOT_EXISTS;


Could you please help to review those Doris changes? @yuqi1129 @zhoukangcn

mchades · 2024-05-10T02:27:38Z

core/src/main/java/com/datastrato/gravitino/storage/relational/service/CatalogMetaService.java

since #3100 merged, this change is no longer needed, right?

jerryshao · 2024-05-10T05:05:01Z

@charliecheng630 would you please rebase the code?

jerryshao · 2024-05-10T06:30:46Z

api/src/main/java/com/datastrato/gravitino/SupportsMetalakes.java

@@ -75,7 +75,7 @@ Metalake alterMetalake(NameIdentifier ident, MetalakeChange... changes)
   * Drop a metalake with specified identifier.
   *
   * @param ident The identifier of the metalake.
-   * @return True if the metalake was dropped, false otherwise.
+   * @return True if the metalake was dropped, false if the metalake does not exist.
   */
  boolean dropMetalake(NameIdentifier ident);


I see you don't change the metalake and catalog related implementations, can you confirm do they have the correct implementation?

jerryshao · 2024-05-10T06:33:51Z

server/src/test/java/com/datastrato/gravitino/server/web/rest/TestMetalakeOperations.java

+    Assertions.assertEquals(Response.Status.OK.getStatusCode(), resp2.getStatus());
+    DropResponse dropResponse2 = resp2.readEntity(DropResponse.class);
+    Assertions.assertEquals(0, dropResponse2.getCode());
+    Assertions.assertFalse(dropResponse2.dropped());


I think you'd better also add a metalake integration test.

jerryshao · 2024-05-10T06:36:25Z

@charliecheng630 can you please also confirm that if anything should be changed in the client side?

.../src/main/java/com/datastrato/gravitino/catalog/doris/converter/DorisExceptionConverter.java

zhoukangcn · 2024-05-10T11:02:40Z

...ql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/CatalogMysqlIT.java

@@ -1264,7 +1264,8 @@ void testNameSpec() {
    Assertions.assertTrue(
        Arrays.stream(schemaIdents).anyMatch(s -> s.name().equals(testSchemaName)));

-    Assertions.assertTrue(catalog.asSchemas().dropSchema(schemaIdent, false));
+    Assertions.assertTrue(
+        catalog.asSchemas().dropSchema(schemaIdent, false), "schema should be dropped");


I notice that schema should be dropped use many times. Do we need to define a function in ITUtils?

mchades · 2024-05-14T01:57:58Z

core/src/main/java/com/datastrato/gravitino/storage/relational/service/CatalogMetaService.java

Are those changes still needed?

Are those changes still needed?

yes.

Maybe not? #3100 did the same things in high level

charliecheng630 · 2024-05-21T15:41:43Z

@mchades Rebased & updated. Please help to review again when you have time, thanks!

mchades · 2024-05-22T13:35:59Z

...g-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalogOperations.java

@@ -239,13 +239,12 @@ public JdbcSchema alterSchema(NameIdentifier ident, SchemaChange... changes)
   *
   * @param ident The identifier of the schema to drop.
   * @param cascade If set to true, drops all the tables in the schema as well.
-   * @return true if the schema was dropped successfully, false otherwise.
+   * @return true if the schema is successfully dropped; false if the table does not exist.


table -> schema

mchades · 2024-05-22T13:53:45Z

.../src/main/java/com/datastrato/gravitino/catalog/doris/converter/DorisExceptionConverter.java

      case CODE_DATABASE_NOT_EXISTS:
        return new NoSuchSchemaException(se, se.getMessage());
+      case CODE_UNKNOWN_DATABASE:
+        return new NoSuchSchemaException(se, se.getMessage());


Can be simplified to：

case CODE_DATABASE_NOT_EXISTS: case CODE_UNKNOWN_DATABASE: return new NoSuchSchemaException(se, se.getMessage());

mchades · 2024-05-22T14:13:11Z

...is/src/test/java/com/datastrato/gravitino/catalog/doris/integration/test/CatalogDorisIT.java

@@ -122,7 +123,10 @@ private void clearTableAndSchema() {
    NameIdentifier[] nameIdentifiers =
        catalog.asTableCatalog().listTables(Namespace.of(metalakeName, catalogName, schemaName));
    for (NameIdentifier nameIdentifier : nameIdentifiers) {
-      catalog.asTableCatalog().dropTable(nameIdentifier);
+      // Currently, tables with a status of SCHEMA_CHANGE are not supported for drop operations.
+      if (!nameIdentifier.name().contains("index")) {


where does the index table come from? I don't see the table creation in test

mchades · 2024-05-22T14:16:05Z

...is/src/test/java/com/datastrato/gravitino/catalog/doris/integration/test/CatalogDorisIT.java

+      // Currently, tables with a status of SCHEMA_CHANGE are not supported for drop operations.
+      if (!nameIdentifier.name().contains("index")) {
+        catalog.asTableCatalog().dropTable(nameIdentifier);
+      }
    }
    catalog.asSchemas().dropSchema(NameIdentifier.of(metalakeName, catalogName, schemaName), true);


since here set cascade=true, the drop table operations above seems unnecessary

mchades · 2024-05-22T14:58:46Z

core/src/main/java/com/datastrato/gravitino/storage/relational/JDBCBackend.java

this file change is unnecessary

mchades self-requested a review April 29, 2024 13:52

mchades assigned charliecheng630 Apr 29, 2024

mchades reviewed May 6, 2024

View reviewed changes

mchades reviewed May 7, 2024

View reviewed changes

charliecheng630 force-pushed the #2434 branch from 9b8f84f to 1c259b3 Compare May 9, 2024 11:45

mchades reviewed May 10, 2024

View reviewed changes

jerryshao reviewed May 10, 2024

View reviewed changes

zhoukangcn reviewed May 10, 2024

View reviewed changes

charliecheng630 force-pushed the #2434 branch from 1c259b3 to c05171c Compare May 13, 2024 14:46

mchades reviewed May 14, 2024

View reviewed changes

charliecheng630 force-pushed the #2434 branch from c05171c to 52371ea Compare May 16, 2024 11:54

charliecheng630 marked this pull request as draft May 16, 2024 15:03

charliecheng630 force-pushed the #2434 branch 2 times, most recently from 8f374e1 to 9f5938d Compare May 19, 2024 04:38

charliecheng630 added 6 commits May 21, 2024 20:08

catalog-hadoop, catalog-hive, catalog-jdbc-common

55115ae

rebase

1145d85

check and fix typo

b346f0f

CatalogMetaService MetalakeMetaService

09b9099

add test

a46334e

fix

87b99b7

charliecheng630 added 13 commits May 21, 2024 20:08

fix

d85247d

fix

95a0275

fix

5a664e3

rebase

3209599

fix

eb82434

fix

36b9826

fix

75b0698

fix

82bedcf

fix

aeb5410

rebase

11d2355

fix

0ede679

fix

6e18065

fix

549860c

charliecheng630 force-pushed the #2434 branch from 5daefec to 549860c Compare May 21, 2024 12:42

charliecheng630 marked this pull request as ready for review May 21, 2024 15:38

mchades reviewed May 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#2434] Improvement: if DropXXX success, return true, if not exists return false #3222

[#2434] Improvement: if DropXXX success, return true, if not exists return false #3222

charliecheng630 commented Apr 29, 2024

jerryshao commented May 6, 2024

mchades commented May 6, 2024

charliecheng630 commented May 7, 2024

charliecheng630 commented May 7, 2024

mchades left a comment

charliecheng630 commented May 9, 2024

charliecheng630 commented May 9, 2024 •

edited

mchades May 10, 2024 •

edited

mchades May 10, 2024

jerryshao commented May 10, 2024

jerryshao May 10, 2024

jerryshao May 10, 2024

jerryshao commented May 10, 2024

zhoukangcn May 10, 2024 •

edited

mchades May 14, 2024

charliecheng630 May 14, 2024

mchades May 14, 2024

charliecheng630 commented May 21, 2024

mchades May 22, 2024

mchades May 22, 2024

mchades May 22, 2024

mchades May 22, 2024

mchades May 22, 2024

[#2434] Improvement: if DropXXX success, return true, if not exists return false #3222

Are you sure you want to change the base?

[#2434] Improvement: if DropXXX success, return true, if not exists return false #3222

Conversation

charliecheng630 commented Apr 29, 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 commented May 6, 2024

mchades commented May 6, 2024

charliecheng630 commented May 7, 2024

charliecheng630 commented May 7, 2024

mchades left a comment

Choose a reason for hiding this comment

charliecheng630 commented May 9, 2024

charliecheng630 commented May 9, 2024 • edited

mchades May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented May 10, 2024

zhoukangcn May 10, 2024 • edited

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

charliecheng630 commented May 21, 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

charliecheng630 commented May 9, 2024 •

edited

mchades May 10, 2024 •

edited

zhoukangcn May 10, 2024 •

edited