yuqi1129 commented on code in PR #13499: URL: https://github.com/apache/gravitino/pull/13499#discussion_r4144307325
########## core/src/main/java/org/apache/gravitino/SupportsConditionalCatalogDelete.java: ########## @@ -0,0 +1,40 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino; + +import java.io.IOException; +import java.util.Set; + +/** + * An optional capability for deleting a catalog after checking its remaining schemas atomically. + */ +public interface SupportsConditionalCatalogDelete { + + /** + * Delete a catalog only if all remaining schemas have IDs in the allowlist. The check and + * deletion must run in one transaction and be serialized with schema creation. + * + * @param ident the catalog identifier + * @param allowedSchemaIds IDs of schemas that may be deleted with the catalog + * @return true if the catalog was deleted + * @throws IOException if the store operation fails + */ + boolean deleteCatalogWithAllowedSchemas(NameIdentifier ident, Set<Long> allowedSchemaIds) Review Comment: Thanks. The SPI Javadoc now requires NonEmptyEntityException for any schema outside the allowlist and reserves false for an already absent catalog. This matches the manager's return-value handling and both implementations. ########## core/src/main/java/org/apache/gravitino/storage/relational/service/CatalogMetaService.java: ########## @@ -272,21 +273,50 @@ public <E extends Entity & HasIdentifier> CatalogEntity updateCatalog( metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME, baseMetricName = "deleteCatalog") public boolean deleteCatalog(NameIdentifier identifier, boolean cascade) { + return deleteCatalog(identifier, cascade, Set.of()); + } + + /** + * Delete a catalog after checking under the catalog row lock that every remaining schema is among + * those classified as safe to discard by the manager. + * + * @param identifier the catalog identifier + * @param allowedSchemaIds IDs of schema entities that may be deleted with the catalog + * @return true if the catalog was deleted + */ + @Monitored( + metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME, + baseMetricName = "deleteCatalog") + public boolean deleteCatalogWithAllowedSchemas( + NameIdentifier identifier, Set<Long> allowedSchemaIds) { + return deleteCatalog(identifier, false, allowedSchemaIds); + } + + private boolean deleteCatalog( + NameIdentifier identifier, boolean cascade, Set<Long> allowedSchemaIds) { NameIdentifierUtil.checkCatalog(identifier); String catalogName = identifier.name(); // Read the whole row, not just the ID, because the delete below needs the version we saw. CatalogPO catalogPO = getCatalogPOByName(identifier.namespace().level(0), catalogName); long catalogId = catalogPO.getCatalogId(); - if (cascade) { + if (cascade || !allowedSchemaIds.isEmpty()) { SessionUtils.doMultipleWithCommit( () -> { // Delete the parent first, then its children. The parent delete locks the catalog row, // and schema writes lock that same row before they touch a schema, so no schema can be // added or removed after this point. Anything that goes wrong later in this // transaction rolls this soft delete back with it. deleteCatalogWithVersion(identifier, catalogPO); + if (!cascade) { + List<SchemaPO> schemaPOs = listSchemaPOsForCascade(catalogId); + if (schemaPOs.stream() + .anyMatch(schema -> !allowedSchemaIds.contains(schema.getSchemaId()))) { + throw new NonEmptyEntityException( + "Entity %s has sub-entities, you should remove sub-entities first", identifier); + } Review Comment: Agreed: the allowlist protects direct child schema IDs, not tables or other descendants created inside an allowed schema. That grandchild race predates this PR and needs a separate write-fencing design. I narrowed the PR description and user-facing claim to direct child catalogs/schemas and explicitly documented this remaining behavior. ########## core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java: ########## @@ -378,14 +378,15 @@ public boolean dropMetalake(NameIdentifier ident, boolean force) "Metalake %s is in use, please disable it first or use force option", ident); } - List<CatalogEntity> catalogEntities = - store.list(Namespace.of(ident.name()), CatalogEntity.class, EntityType.CATALOG); - if (!catalogEntities.isEmpty() && !force) { + if (force) { + return store.delete(ident, EntityType.METALAKE, true); + } + try { + return store.delete(ident, EntityType.METALAKE, false); Review Comment: Good catch. I restored the catalog-scoped sweep on both metalake delete paths and the orphan-schema cleanup after the non-force catalog-emptiness check. A new regression test leaves schema/table metadata under a soft-deleted catalog and verifies non-force metalake deletion clears the schema row and table-version rows on H2, MySQL, and PostgreSQL. ########## core/src/main/java/org/apache/gravitino/storage/relational/service/CatalogMetaService.java: ########## @@ -272,21 +273,50 @@ public <E extends Entity & HasIdentifier> CatalogEntity updateCatalog( metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME, baseMetricName = "deleteCatalog") public boolean deleteCatalog(NameIdentifier identifier, boolean cascade) { + return deleteCatalog(identifier, cascade, Set.of()); + } + + /** + * Delete a catalog after checking under the catalog row lock that every remaining schema is among + * those classified as safe to discard by the manager. + * + * @param identifier the catalog identifier + * @param allowedSchemaIds IDs of schema entities that may be deleted with the catalog + * @return true if the catalog was deleted + */ + @Monitored( + metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME, + baseMetricName = "deleteCatalog") + public boolean deleteCatalogWithAllowedSchemas( + NameIdentifier identifier, Set<Long> allowedSchemaIds) { + return deleteCatalog(identifier, false, allowedSchemaIds); + } + + private boolean deleteCatalog( + NameIdentifier identifier, boolean cascade, Set<Long> allowedSchemaIds) { NameIdentifierUtil.checkCatalog(identifier); String catalogName = identifier.name(); // Read the whole row, not just the ID, because the delete below needs the version we saw. CatalogPO catalogPO = getCatalogPOByName(identifier.namespace().level(0), catalogName); long catalogId = catalogPO.getCatalogId(); - if (cascade) { + if (cascade || !allowedSchemaIds.isEmpty()) { SessionUtils.doMultipleWithCommit( () -> { // Delete the parent first, then its children. The parent delete locks the catalog row, // and schema writes lock that same row before they touch a schema, so no schema can be // added or removed after this point. Anything that goes wrong later in this // transaction rolls this soft delete back with it. deleteCatalogWithVersion(identifier, catalogPO); + if (!cascade) { + List<SchemaPO> schemaPOs = listSchemaPOsForCascade(catalogId); Review Comment: Fixed. The transaction now reads the schema list once after locking the catalog row, checks that snapshot against the allowlist, and passes the same list to deleteSchemasWithVersions. ########## core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java: ########## @@ -311,6 +316,23 @@ public boolean delete(NameIdentifier ident, Entity.EntityType entityType, boolea } } + @Override + public boolean deleteCatalogWithAllowedSchemas(NameIdentifier ident, Set<Long> allowedSchemaIds) + throws IOException { + if (!(backend instanceof SupportsConditionalCatalogDelete)) { + throw new UnsupportedOperationException( + "Atomic catalog delete with allowed schemas is not supported by this backend"); Review Comment: Fixed. The unsupported-backend error now names the catalog and backend and suggests the force option or a backend implementing SupportsConditionalCatalogDelete. The test checks the actionable message. ########## core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java: ########## @@ -1414,7 +1415,38 @@ public boolean dropCatalog(NameIdentifier ident, boolean force) // the cache with stale data between invalidate and delete. Map<String, String> catalogProperties = copyProperties(catalogWrapper.catalog().entity().getProperties()); - boolean deleted = store.delete(ident, EntityType.CATALOG, true); + boolean deleted; + if (force) { + deleted = store.delete(ident, EntityType.CATALOG, true); + } else { + try { + if (schemaEntities.isEmpty()) { + deleted = store.delete(ident, EntityType.CATALOG, false); + } else { + Set<Long> allowedSchemaIds = + schemaEntities.stream().map(SchemaEntity::id).collect(Collectors.toSet()); + if (!(store instanceof SupportsConditionalCatalogDelete)) { + // Fail closed: an unconditional cascade could delete a schema created after + // the classification above. + throw new UnsupportedOperationException( + String.format( + "Catalog %s still has built-in, imported, or externally removed " + + "schemas, and entity store %s cannot delete it atomically with " + + "them. Use the force option, or use an entity store that " + + "implements %s", + ident, + store.getClass().getName(), + SupportsConditionalCatalogDelete.class.getSimpleName())); + } + deleted = + ((SupportsConditionalCatalogDelete) store) + .deleteCatalogWithAllowedSchemas(ident, allowedSchemaIds); + } + } catch (NonEmptyEntityException e) { + throw new NonEmptyCatalogException( + "Catalog %s has schemas, please drop them first or use force option", ident); Review Comment: Fixed. NonEmptyCatalogException now has a cause-taking constructor, and CatalogManager preserves the store exception. The relational and in-memory stores name the unexpected schema in that cause; manager and service regression tests assert the detail survives. -- 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]
