This is an automated email from the ASF dual-hosted git repository.
mchades pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new b7554295d [#3685] Improvement: revisit all the purgeXXX
implementations to have a consistent behavior (#4091)
b7554295d is described below
commit b7554295d2c76e200393319e1ef1b49a1f5e9e17
Author: Ziva Li <[email protected]>
AuthorDate: Tue Jul 9 22:31:06 2024 +0800
[#3685] Improvement: revisit all the purgeXXX implementations to have a
consistent behavior (#4091)
### What changes were proposed in this pull request?
When purging tables, return `True` if the target was purged, and `False`
if the target does not exist.
### Why are the changes needed?
Fix: #3685
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
ITs and UTs.
---
.../gravitino/rel/SupportsPartitions.java | 8 +++----
.../com/datastrato/gravitino/rel/TableCatalog.java | 4 ++--
.../catalog/hive/HiveCatalogOperations.java | 12 ++++++----
.../gravitino/catalog/hive/TestHiveTable.java | 4 ++++
.../hive/integration/test/CatalogHiveIT.java | 4 ++++
.../catalog/jdbc/JdbcCatalogOperations.java | 3 +--
.../jdbc/operation/JdbcTableOperations.java | 11 ++++++++-
.../catalog/jdbc/operation/TableOperation.java | 2 +-
.../iceberg/IcebergCatalogOperations.java | 2 ++
.../gravitino/client/RelationalCatalog.java | 26 +++++++++-------------
.../gravitino/client/TestRelationalCatalog.java | 5 ++++-
.../gravitino/catalog/PartitionDispatcher.java | 2 +-
.../catalog/TableOperationDispatcher.java | 2 +-
13 files changed, 51 insertions(+), 34 deletions(-)
diff --git
a/api/src/main/java/com/datastrato/gravitino/rel/SupportsPartitions.java
b/api/src/main/java/com/datastrato/gravitino/rel/SupportsPartitions.java
index adcafc648..802011b6f 100644
--- a/api/src/main/java/com/datastrato/gravitino/rel/SupportsPartitions.java
+++ b/api/src/main/java/com/datastrato/gravitino/rel/SupportsPartitions.java
@@ -94,7 +94,7 @@ public interface SupportsPartitions {
* Drop a partition with specified name.
*
* @param partitionName the name of the partition
- * @return true if a partition was deleted successfully, false if the
partition does not exist.
+ * @return true if the partition is deleted successfully, false if the
partition does not exist.
*/
boolean dropPartition(String partitionName);
@@ -104,12 +104,10 @@ public interface SupportsPartitions {
* purging partitions, {@link UnsupportedOperationException} is thrown.
*
* @param partitionName The name of the partition.
- * @return true if a partition was deleted, false if the partition did not
exist.
- * @throws NoSuchPartitionException If the partition does not exist.
+ * @return true if the partition is purged, false if the partition does not
exist.
* @throws UnsupportedOperationException If partition purging is not
supported.
*/
- default boolean purgePartition(String partitionName)
- throws NoSuchPartitionException, UnsupportedOperationException {
+ default boolean purgePartition(String partitionName) throws
UnsupportedOperationException {
throw new UnsupportedOperationException("Partition purging is not
supported");
}
}
diff --git a/api/src/main/java/com/datastrato/gravitino/rel/TableCatalog.java
b/api/src/main/java/com/datastrato/gravitino/rel/TableCatalog.java
index a0ab16e00..d6f67920b 100644
--- a/api/src/main/java/com/datastrato/gravitino/rel/TableCatalog.java
+++ b/api/src/main/java/com/datastrato/gravitino/rel/TableCatalog.java
@@ -268,7 +268,7 @@ public interface TableCatalog {
* is removed.
*
* @param ident A table identifier.
- * @return True if the table was dropped, false if the table did not exist.
+ * @return True if the table is dropped, false if the table does not exist.
*/
boolean dropTable(NameIdentifier ident);
@@ -282,7 +282,7 @@ public interface TableCatalog {
* implementation throws an {@link UnsupportedOperationException}.
*
* @param ident A table identifier.
- * @return True if the table was purged, false if the table did not exist.
+ * @return True if the table is purged, false if the table does not exist.
* @throws UnsupportedOperationException If the catalog does not support to
purge a table.
*/
default boolean purgeTable(NameIdentifier ident) throws
UnsupportedOperationException {
diff --git
a/catalogs/catalog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveCatalogOperations.java
b/catalogs/catalog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveCatalogOperations.java
index c0b886539..bf1340858 100644
---
a/catalogs/catalog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveCatalogOperations.java
+++
b/catalogs/catalog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveCatalogOperations.java
@@ -1080,10 +1080,14 @@ public class HiveCatalogOperations implements
CatalogOperations, SupportsSchemas
*/
@Override
public boolean purgeTable(NameIdentifier tableIdent) throws
UnsupportedOperationException {
- if (isExternalTable(tableIdent)) {
- throw new UnsupportedOperationException("Can't purge a external hive
table");
- } else {
- return dropHiveTable(tableIdent, true, true);
+ try {
+ if (isExternalTable(tableIdent)) {
+ throw new UnsupportedOperationException("Can't purge a external hive
table");
+ } else {
+ return dropHiveTable(tableIdent, true, true);
+ }
+ } catch (NoSuchTableException e) {
+ return false;
}
}
diff --git
a/catalogs/catalog-hive/src/test/java/com/datastrato/gravitino/catalog/hive/TestHiveTable.java
b/catalogs/catalog-hive/src/test/java/com/datastrato/gravitino/catalog/hive/TestHiveTable.java
index 63ffd452b..5339e1fad 100644
---
a/catalogs/catalog-hive/src/test/java/com/datastrato/gravitino/catalog/hive/TestHiveTable.java
+++
b/catalogs/catalog-hive/src/test/java/com/datastrato/gravitino/catalog/hive/TestHiveTable.java
@@ -549,6 +549,10 @@ public class TestHiveTable extends
MiniHiveMetastoreService {
Assertions.assertTrue(hiveCatalogOperations.tableExists(tableIdentifier));
hiveCatalogOperations.purgeTable(tableIdentifier);
Assertions.assertFalse(hiveCatalogOperations.tableExists(tableIdentifier));
+ // purging non-exist table should return false
+ Assertions.assertFalse(
+ hiveCatalogOperations.purgeTable(tableIdentifier),
+ "The table should not be found in the catalog");
}
@Test
diff --git
a/catalogs/catalog-hive/src/test/java/com/datastrato/gravitino/catalog/hive/integration/test/CatalogHiveIT.java
b/catalogs/catalog-hive/src/test/java/com/datastrato/gravitino/catalog/hive/integration/test/CatalogHiveIT.java
index fe3b374d0..2b3fade44 100644
---
a/catalogs/catalog-hive/src/test/java/com/datastrato/gravitino/catalog/hive/integration/test/CatalogHiveIT.java
+++
b/catalogs/catalog-hive/src/test/java/com/datastrato/gravitino/catalog/hive/integration/test/CatalogHiveIT.java
@@ -1522,6 +1522,10 @@ public class CatalogHiveIT extends AbstractIT {
catalog.asTableCatalog().purgeTable(NameIdentifier.of(schemaName,
tableName));
Boolean existed = hiveClientPool.run(client ->
client.tableExists(schemaName, tableName));
Assertions.assertFalse(existed, "The Hive table should not exist");
+ // purging non-exist table should return false
+ Assertions.assertFalse(
+ catalog.asTableCatalog().purgeTable(NameIdentifier.of(schemaName,
tableName)),
+ "The table should not be found in the catalog");
Path tableDirectory = new Path(hiveTab.getSd().getLocation());
Assertions.assertFalse(hdfs.exists(tableDirectory), "The table directory
should not exist");
Path trashDirectory = hdfs.getTrashRoot(tableDirectory);
diff --git
a/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalogOperations.java
b/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalogOperations.java
index 135952c26..e8bc96076 100644
---
a/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalogOperations.java
+++
b/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/JdbcCatalogOperations.java
@@ -442,8 +442,7 @@ public class JdbcCatalogOperations implements
CatalogOperations, SupportsSchemas
@Override
public boolean purgeTable(NameIdentifier tableIdent) throws
UnsupportedOperationException {
String databaseName =
NameIdentifier.of(tableIdent.namespace().levels()).name();
- tableOperation.purge(databaseName, tableIdent.name());
- return true;
+ return tableOperation.purge(databaseName, tableIdent.name());
}
/**
diff --git
a/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcTableOperations.java
b/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcTableOperations.java
index 39b6860a1..a93b50216 100644
---
a/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcTableOperations.java
+++
b/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcTableOperations.java
@@ -266,7 +266,16 @@ public abstract class JdbcTableOperations implements
TableOperation {
}
@Override
- public void purge(String databaseName, String tableName) throws
NoSuchTableException {
+ public boolean purge(String databaseName, String tableName) {
+ try {
+ purgeTable(databaseName, tableName);
+ } catch (NoSuchTableException | NoSuchSchemaException e) {
+ return false;
+ }
+ return true;
+ }
+
+ protected void purgeTable(String databaseName, String tableName) {
LOG.info("Attempting to purge table {} from database {}", tableName,
databaseName);
try (Connection connection = getConnection(databaseName)) {
JdbcConnectorUtils.executeUpdate(connection,
generatePurgeTableSql(tableName));
diff --git
a/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/TableOperation.java
b/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/TableOperation.java
index 987d57207..5e0d19223 100644
---
a/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/TableOperation.java
+++
b/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/TableOperation.java
@@ -113,5 +113,5 @@ public interface TableOperation {
* @param databaseName The name of the database.
* @param tableName The name of the table.
*/
- void purge(String databaseName, String tableName) throws
NoSuchTableException;
+ boolean purge(String databaseName, String tableName);
}
diff --git
a/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java
b/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java
index fc19399b0..bcd5917e7 100644
---
a/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java
+++
b/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java
@@ -547,6 +547,8 @@ public class IcebergCatalogOperations implements
CatalogOperations, SupportsSche
} catch (org.apache.iceberg.exceptions.NoSuchTableException e) {
LOG.warn("Iceberg table {} does not exist", tableIdent.name());
return false;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
}
}
diff --git
a/clients/client-java/src/main/java/com/datastrato/gravitino/client/RelationalCatalog.java
b/clients/client-java/src/main/java/com/datastrato/gravitino/client/RelationalCatalog.java
index 96521f195..f6132583e 100644
---
a/clients/client-java/src/main/java/com/datastrato/gravitino/client/RelationalCatalog.java
+++
b/clients/client-java/src/main/java/com/datastrato/gravitino/client/RelationalCatalog.java
@@ -237,7 +237,7 @@ public class RelationalCatalog extends BaseSchemaCatalog
implements TableCatalog
* Purge the table with specified identifier.
*
* @param ident The identifier of the table, which should be "schema.table"
format.
- * @return true if the table is purged successfully, false otherwise.
+ * @return true if the table is purged successfully, false if the table does
not exist.
*/
@Override
public boolean purgeTable(NameIdentifier ident) throws
UnsupportedOperationException {
@@ -246,21 +246,15 @@ public class RelationalCatalog extends BaseSchemaCatalog
implements TableCatalog
Namespace fullNamespace = getTableFullNamespace(ident.namespace());
Map<String, String> params = new HashMap<>();
params.put("purge", "true");
- try {
- DropResponse resp =
- restClient.delete(
- formatTableRequestPath(fullNamespace) + "/" +
RESTUtils.encodeString(ident.name()),
- params,
- DropResponse.class,
- Collections.emptyMap(),
- ErrorHandlers.tableErrorHandler());
- resp.validate();
- return resp.dropped();
- } catch (UnsupportedOperationException e) {
- throw e;
- } catch (Exception e) {
- return false;
- }
+ DropResponse resp =
+ restClient.delete(
+ formatTableRequestPath(fullNamespace) + "/" +
RESTUtils.encodeString(ident.name()),
+ params,
+ DropResponse.class,
+ Collections.emptyMap(),
+ ErrorHandlers.tableErrorHandler());
+ resp.validate();
+ return resp.dropped();
}
@VisibleForTesting
diff --git
a/clients/client-java/src/test/java/com/datastrato/gravitino/client/TestRelationalCatalog.java
b/clients/client-java/src/test/java/com/datastrato/gravitino/client/TestRelationalCatalog.java
index 87781850c..839082e41 100644
---
a/clients/client-java/src/test/java/com/datastrato/gravitino/client/TestRelationalCatalog.java
+++
b/clients/client-java/src/test/java/com/datastrato/gravitino/client/TestRelationalCatalog.java
@@ -1122,7 +1122,10 @@ public class TestRelationalCatalog extends TestBase {
ErrorResponse errorResp = ErrorResponse.internalError("internal error");
buildMockResource(Method.DELETE, tablePath, null, errorResp,
SC_INTERNAL_SERVER_ERROR);
- Assertions.assertFalse(catalog.asTableCatalog().purgeTable(tableId));
+ Throwable exception =
+ Assertions.assertThrows(
+ RuntimeException.class, () ->
catalog.asTableCatalog().purgeTable(tableId));
+ Assertions.assertTrue(exception.getMessage().contains("internal error"));
}
@Test
diff --git
a/core/src/main/java/com/datastrato/gravitino/catalog/PartitionDispatcher.java
b/core/src/main/java/com/datastrato/gravitino/catalog/PartitionDispatcher.java
index e6bb15b09..ee743d9b1 100644
---
a/core/src/main/java/com/datastrato/gravitino/catalog/PartitionDispatcher.java
+++
b/core/src/main/java/com/datastrato/gravitino/catalog/PartitionDispatcher.java
@@ -99,7 +99,7 @@ public interface PartitionDispatcher {
*
* @param tableIdent The identifier of the table.
* @param partitionName The name of the partition.
- * @return True if the partition was purged, false if the partition does not
exist.
+ * @return True if the partition is purged, false if the partition does not
exist.
* @throws UnsupportedOperationException If partition purging is not
supported.
*/
default boolean purgePartition(NameIdentifier tableIdent, String
partitionName)
diff --git
a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java
b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java
index df90e65db..406a62d3d 100644
---
a/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java
+++
b/core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java
@@ -347,7 +347,7 @@ public class TableOperationDispatcher extends
OperationDispatcher implements Tab
* implementation throws an {@link UnsupportedOperationException}.
*
* @param ident A table identifier.
- * @return True if the table was purged, false if the table did not exist.
+ * @return True if the table is purged, false if the table does not exist.
* @throws UnsupportedOperationException If the catalog does not support to
purge a table.
* @throws RuntimeException If an error occurs while purging the table.
*/
