New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#2434] Improvement: if DropXXX success, return true, if not exists return false #3222
base: main
Are you sure you want to change the base?
Conversation
@mchades would you please help to review this? |
.../catalog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveCatalogOperations.java
Outdated
Show resolved
Hide resolved
...-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/DatabaseOperation.java
Show resolved
Hide resolved
...dbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/TableOperation.java
Show resolved
Hide resolved
...on/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcDatabaseOperations.java
Outdated
Show resolved
Hide resolved
...ommon/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcTableOperations.java
Outdated
Show resolved
Hide resolved
...g-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalogOperations.java
Show resolved
Hide resolved
Are the changes in |
Yes. |
Update. Please help to review again when you have time~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
...ommon/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcTableOperations.java
Outdated
Show resolved
Hide resolved
...on/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcDatabaseOperations.java
Outdated
Show resolved
Hide resolved
Updated. Please help to review again when you have time, thanks! |
@mchades Updated. Please help to review again when you have time, thanks! |
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please help to review those Doris changes? @yuqi1129 @zhoukangcn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since #3100 merged, this change is no longer needed, right?
@charliecheng630 would you please rebase the code? |
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you don't change the metalake and catalog related implementations, can you confirm do they have the correct implementation?
Assertions.assertEquals(Response.Status.OK.getStatusCode(), resp2.getStatus()); | ||
DropResponse dropResponse2 = resp2.readEntity(DropResponse.class); | ||
Assertions.assertEquals(0, dropResponse2.getCode()); | ||
Assertions.assertFalse(dropResponse2.dropped()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you'd better also add a metalake integration test.
@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
Outdated
Show resolved
Hide resolved
@@ -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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice that schema should be dropped
use many times. Do we need to define a function in ITUtils?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are those changes still needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are those changes still needed?
yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not? #3100 did the same things in high level
8f374e1
to
9f5938d
Compare
@mchades Rebased & updated. Please help to review again when you have time, thanks! |
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
table -> schema
case CODE_DATABASE_NOT_EXISTS: | ||
return new NoSuchSchemaException(se, se.getMessage()); | ||
case CODE_UNKNOWN_DATABASE: | ||
return new NoSuchSchemaException(se, se.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be simplified to:
case CODE_DATABASE_NOT_EXISTS:
case CODE_UNKNOWN_DATABASE:
return new NoSuchSchemaException(se, se.getMessage());
@@ -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")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where does the index
table come from? I don't see the table creation in test
// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since here set cascade=true
, the drop table operations above seems unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file change is unnecessary
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