diqiu50 commented on code in PR #12235:
URL: https://github.com/apache/gravitino/pull/12235#discussion_r3759747891
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -374,20 +395,17 @@ public boolean dropTable(NameIdentifier ident) {
return droppedFromCatalog;
}
- // For unmanaged table, it could happen that the table:
- // 1. Is not found in the catalog (dropped directly from underlying
sources)
- // 2. Is found in the catalog but not in the store (not managed by
Gravitino)
- // 3. Is found in the catalog and the store (managed by Gravitino)
- // 4. Neither found in the catalog nor in the store.
- // In all situations, we try to delete the table from the store, but
we don't take the
- // return value of the store operation into account. We only take
the return value of the
- // catalog into account.
- try {
- store.delete(ident, TABLE);
- } catch (NoSuchEntityException e) {
- LOG.warn("The table to be dropped does not exist in the store:
{}", ident, e);
- } catch (Exception e) {
- throw new RuntimeException(e);
+ // A false result can mean that a concurrent rename already moved
the external table.
+ // Only remove the stored registration after the catalog confirms
that this drop deleted
+ // the table.
+ if (droppedFromCatalog) {
Review Comment:
The same issue
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -338,6 +350,15 @@ public Table alterTable(NameIdentifier ident,
TableChange... changes)
"UPDATE",
tableId);
+ if (isRenameTable && updatedTableEntity == null) {
Review Comment:
Is using updatedTableEntity == null to determine whether the store update
failed accurate?
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -339,20 +339,17 @@ public boolean dropSchema(NameIdentifier ident, boolean
cascade) throws NonEmpty
return droppedFromCatalog;
}
- // For the unmanaged schema, it could happen that the schema:
- // 1. It's not found in the catalog (dropped directly from
underlying sources)
- // 2. It's found in the catalog but not in the store (not managed by
Gravitino)
- // 3. It's found in the catalog and the store (managed by Gravitino)
- // 4. Neither found in the catalog nor in the store.
- // In all situations, we try to delete the schema from the store,
but we don't take the
- // return value of the store operation into account. We only take
the return value of the
- // catalog into account.
- try {
- store.delete(ident, SCHEMA, true);
- } catch (NoSuchEntityException e) {
- LOG.warn("The schema to be dropped does not exist in the store:
{}", ident, e);
- } catch (Exception e) {
- throw new RuntimeException(e);
+ // A false result can mean that the schema was renamed directly in
the external catalog.
+ // Only remove the stored registration after the catalog confirms
that this drop deleted
+ // the schema and its descendants.
+ if (droppedFromCatalog) {
Review Comment:
Once the external table is gone, dropTable always returns false, so the
stored registration is never deleted — and there is no API left that can clean
it up.
--
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]
