jerryshao commented on code in PR #14120:
URL: https://github.com/apache/gravitino/pull/14120#discussion_r4345762872
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -925,71 +895,32 @@ public boolean dropSchema(NameIdentifier ident, boolean
cascade) throws NonEmpty
Optional.ofNullable(schemaEntity.properties()).orElse(Collections.emptyMap());
Map<String, Path> schemaPaths = getAndCheckSchemaPaths(ident.name(),
properties);
- boolean dropped = super.dropSchema(ident, cascade);
- if (disableFSOps) {
- // No FS cleanup; still remove fileset write-through secrets when
cascade deleted them.
- // Schema secrets are cleaned by SchemaOperationDispatcher.
- if (dropped && cascade) {
- for (FilesetEntity fileset : filesets) {
- secretManager.deleteSecretsFromProperties(fileset.properties());
- }
+ // Catalog force-drop reaches this method without
SchemaOperationDispatcher's fileset
+ // cleanup. Drop each fileset before its schema so a storage failure
preserves the failed
+ // fileset and its parent for retry. Stop at the first failure; earlier
drops stay committed.
+ if (cascade) {
+ for (FilesetEntity fileset : filesets) {
+ dropFilesetAndGet(fileset.nameIdentifier())
+ .ifPresent(
+ deleted ->
secretManager.deleteSecretsFromProperties(deleted.properties()));
+ }
+
+ // Functions can also live in fileset catalogs. Drop their metadata
explicitly before
Review Comment:
Function rows are deleted before `super.dropSchema(ident, false)`. A fileset
created after the fileset snapshot correctly makes that call throw
`NonEmptySchemaException` and keeps the schema, but the functions are already
gone. `testDropSchemaRejectsFilesetCreatedAfterListing` does not register a
function, so this is untested. Please delete functions only after new filesets
can no longer appear, or in the same transaction as the schema delete, and
extend that test to assert the function is still there when the schema drop is
rejected.
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -740,38 +741,7 @@ public Fileset alterFileset(NameIdentifier ident,
FilesetChange... changes)
@Override
public boolean dropFileset(NameIdentifier ident) {
- try {
- // The relational store runs this cleanup after the metadata CAS wins
but before committing
- // its transaction. The callback therefore sees the exact deleted
snapshot, and an I/O
- // failure can still restore the metadata so the caller may fix
permissions and retry.
- //
- // The price is that the recursive storage delete runs inside that
transaction, holding the
- // fileset rows and a pooled connection for as long as the filesystem
takes. Dropping a
- // fileset with a very large tree is therefore a slow write for that
fileset, and enough
- // concurrent drops can hold up the connection pool.
- Optional<FilesetEntity> deletedFileset =
- store.deleteAndGet(
- ident,
- Entity.EntityType.FILESET,
- FilesetEntity.class,
- filesetEntity -> {
- if (!disableFSOps && filesetEntity.filesetType() ==
Fileset.Type.MANAGED) {
- try {
- deleteManagedFilesetStorage(ident, filesetEntity);
- } catch (IOException ioe) {
- throw new UncheckedIOException(ioe);
- }
- }
- });
- return deletedFileset.isPresent();
- } catch (NoSuchEntityException ne) {
- LOG.warn("Fileset {} does not exist", ident);
- return false;
- } catch (UncheckedIOException uioe) {
- throw ExceptionMessages.wrap("Failed to delete fileset " + ident,
uioe.getCause());
- } catch (IOException ioe) {
- throw ExceptionMessages.wrap("Failed to delete fileset " + ident, ioe);
- }
+ return dropFilesetAndGet(ident).isPresent();
Review Comment:
The deleted-snapshot secret cleanup in `dropSchema` runs only for filesets
that this method itself deletes. Public schema cascade drops filesets earlier
in `SchemaOperationDispatcher` / `FilesetOperationDispatcher`, and
`dropFileset` now discards the `dropFilesetAndGet` snapshot and returns a
boolean. The dispatcher still deletes secrets from the `loadFileset()`
properties captured before the compare-and-set. A secret changed between that
load and the committed delete is not cleaned, and `dropSchema` will not see the
fileset on its second list. Catalog and metalake force-drop bypass the
dispatcher, so they are fine. Please clean secrets from the snapshot inside
`dropFilesetAndGet` and remove the pre-drop cleanup in the dispatcher.
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -925,71 +895,32 @@ public boolean dropSchema(NameIdentifier ident, boolean
cascade) throws NonEmpty
Optional.ofNullable(schemaEntity.properties()).orElse(Collections.emptyMap());
Map<String, Path> schemaPaths = getAndCheckSchemaPaths(ident.name(),
properties);
- boolean dropped = super.dropSchema(ident, cascade);
- if (disableFSOps) {
- // No FS cleanup; still remove fileset write-through secrets when
cascade deleted them.
- // Schema secrets are cleaned by SchemaOperationDispatcher.
- if (dropped && cascade) {
- for (FilesetEntity fileset : filesets) {
- secretManager.deleteSecretsFromProperties(fileset.properties());
- }
+ // Catalog force-drop reaches this method without
SchemaOperationDispatcher's fileset
+ // cleanup. Drop each fileset before its schema so a storage failure
preserves the failed
+ // fileset and its parent for retry. Stop at the first failure; earlier
drops stay committed.
+ if (cascade) {
+ for (FilesetEntity fileset : filesets) {
+ dropFilesetAndGet(fileset.nameIdentifier())
+ .ifPresent(
+ deleted ->
secretManager.deleteSecretsFromProperties(deleted.properties()));
+ }
+
+ // Functions can also live in fileset catalogs. Drop their metadata
explicitly before
+ // the non-cascading schema delete so it still protects any
unprocessed filesets.
+ for (FunctionEntity function :
Review Comment:
`super.dropSchema(ident, false)` only succeeds when `checkSchemaIsEmpty`
sees no tables, views, filesets, functions, models, topics, semantic models, or
descendant schemas. This method now deletes only filesets and functions first.
Any other child is left in place, but the filesets and functions are already
committed, and the caller gets `NonEmptySchemaException` on every retry. The
previous `cascade=true` store delete removed those rows in the same transaction
as the schema. Fileset catalogs cannot create most of those types through their
own APIs, but a stray row would wedge the drop with no catalog API to remove
it. Please reject that case before deleting filesets and functions, or delete
those child types explicitly. Please do not switch the final call back to
`cascade=true`; that would again drop a fileset created after the snapshot
without storage cleanup.
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -925,71 +895,32 @@ public boolean dropSchema(NameIdentifier ident, boolean
cascade) throws NonEmpty
Optional.ofNullable(schemaEntity.properties()).orElse(Collections.emptyMap());
Map<String, Path> schemaPaths = getAndCheckSchemaPaths(ident.name(),
properties);
- boolean dropped = super.dropSchema(ident, cascade);
- if (disableFSOps) {
- // No FS cleanup; still remove fileset write-through secrets when
cascade deleted them.
- // Schema secrets are cleaned by SchemaOperationDispatcher.
- if (dropped && cascade) {
- for (FilesetEntity fileset : filesets) {
- secretManager.deleteSecretsFromProperties(fileset.properties());
- }
+ // Catalog force-drop reaches this method without
SchemaOperationDispatcher's fileset
+ // cleanup. Drop each fileset before its schema so a storage failure
preserves the failed
+ // fileset and its parent for retry. Stop at the first failure; earlier
drops stay committed.
+ if (cascade) {
+ for (FilesetEntity fileset : filesets) {
+ dropFilesetAndGet(fileset.nameIdentifier())
+ .ifPresent(
+ deleted ->
secretManager.deleteSecretsFromProperties(deleted.properties()));
+ }
+
+ // Functions can also live in fileset catalogs. Drop their metadata
explicitly before
+ // the non-cascading schema delete so it still protects any
unprocessed filesets.
+ for (FunctionEntity function :
+ store.list(filesetNs, FunctionEntity.class,
Entity.EntityType.FUNCTION)) {
+ store.delete(function.nameIdentifier(), Entity.EntityType.FUNCTION);
}
- return dropped;
}
+ // Reject remaining filesets instead of deleting their metadata without
storage cleanup.
+ boolean dropped = super.dropSchema(ident, false);
Review Comment:
Schema-directory cleanup still runs only after `super.dropSchema(ident,
false)` has committed (lines 942-963). A false `fs.delete` is logged and
ignored, and an `IOException` is thrown only once the schema row is gone.
Catalog force-drop then skips `deleteSecretsFromProperties` for that schema and
aborts, so a retry cannot remove the empty directory or those schema secrets.
Fileset storage failure now preserves metadata; this path does not. Please
either move empty-directory deletion ahead of the schema commit, or keep the
schema row when directory cleanup fails. If this stays best-effort, delete the
schema's write-through secrets before directory cleanup and do not fail the
catalog drop after the row is already gone.
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -925,71 +895,32 @@ public boolean dropSchema(NameIdentifier ident, boolean
cascade) throws NonEmpty
Optional.ofNullable(schemaEntity.properties()).orElse(Collections.emptyMap());
Map<String, Path> schemaPaths = getAndCheckSchemaPaths(ident.name(),
properties);
- boolean dropped = super.dropSchema(ident, cascade);
- if (disableFSOps) {
- // No FS cleanup; still remove fileset write-through secrets when
cascade deleted them.
- // Schema secrets are cleaned by SchemaOperationDispatcher.
- if (dropped && cascade) {
- for (FilesetEntity fileset : filesets) {
- secretManager.deleteSecretsFromProperties(fileset.properties());
- }
+ // Catalog force-drop reaches this method without
SchemaOperationDispatcher's fileset
+ // cleanup. Drop each fileset before its schema so a storage failure
preserves the failed
+ // fileset and its parent for retry. Stop at the first failure; earlier
drops stay committed.
+ if (cascade) {
+ for (FilesetEntity fileset : filesets) {
Review Comment:
Question: Reusing `deleteAndGet` means each managed fileset holds a pooled
connection for its whole recursive delete, and the schema loop does that one
fileset at a time. The old path deleted locations in parallel after the
metadata transaction. This matches single-fileset drop and is what makes a
storage failure roll the fileset row back. Is the loss of parallelism and the
longer connection hold accepted for schema and catalog force-drop of many large
trees?
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -867,6 +876,249 @@ public void testDropSchemaWithFSOpsDisabled() throws
IOException {
}
}
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ void testDropSchemaWithFunctions(boolean withFileset) throws IOException {
+ String schemaName = "schema_cascade_functions_" + generateTestId();
+ String catalogPath = TEST_ROOT_PATH + "/" + schemaName;
+ createSchema(schemaName, "comment", catalogPath, null);
+ NameIdentifier schemaIdent = NameIdentifier.of("m1", "c1", schemaName);
+ NameIdentifier functionIdent = NameIdentifier.of("m1", "c1", schemaName,
"function");
+ createFunction(functionIdent);
+ if (withFileset) {
+ createFileset("fileset", schemaName, "comment", Fileset.Type.MANAGED,
catalogPath, null);
+ }
+ NameIdentifier filesetIdent = NameIdentifier.of("m1", "c1", schemaName,
"fileset");
+ Path schemaPath = new Path(catalogPath, schemaName);
+ FileSystem fs = schemaPath.getFileSystem(new Configuration());
+
+ try (FilesetCatalogOperations ops = new FilesetCatalogOperations(store,
secretManager)) {
+ ops.initialize(
+ ImmutableMap.of(LOCATION, catalogPath),
+ randomCatalogInfo("m1", "c1"),
+ FILESET_PROPERTIES_METADATA);
+ Assertions.assertThrows(
+ NonEmptySchemaException.class, () -> ops.dropSchema(schemaIdent,
false));
+ Assertions.assertTrue(store.exists(schemaIdent,
Entity.EntityType.SCHEMA));
+ Assertions.assertTrue(store.exists(functionIdent,
Entity.EntityType.FUNCTION));
+ Assertions.assertEquals(withFileset, store.exists(filesetIdent,
Entity.EntityType.FILESET));
+ Assertions.assertTrue(fs.exists(schemaPath));
+
+ Assertions.assertTrue(ops.dropSchema(schemaIdent, true));
+ Assertions.assertFalse(store.exists(schemaIdent,
Entity.EntityType.SCHEMA));
+ Assertions.assertFalse(store.exists(functionIdent,
Entity.EntityType.FUNCTION));
+ Assertions.assertFalse(store.exists(filesetIdent,
Entity.EntityType.FILESET));
+ Assertions.assertFalse(fs.exists(schemaPath));
+ }
+ }
+
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ void testDropSchemaStopsOnStorageFailureAndCanRetry(boolean throwException)
throws IOException {
Review Comment:
nit: The new cascade tests cover a single location that returns false or
throws, plus retry, secrets, external filesets, and `disableFSOps`. Please add
a managed fileset with two locations where the second `delete` fails: the
fileset row and the remaining location should stay, the first location may
already be gone, and a retry should finish the drop. That is the
partial-location behavior `deleteManagedFilesetStorage` already has for a
direct fileset drop, and this path now depends on it.
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -925,71 +895,32 @@ public boolean dropSchema(NameIdentifier ident, boolean
cascade) throws NonEmpty
Optional.ofNullable(schemaEntity.properties()).orElse(Collections.emptyMap());
Map<String, Path> schemaPaths = getAndCheckSchemaPaths(ident.name(),
properties);
- boolean dropped = super.dropSchema(ident, cascade);
- if (disableFSOps) {
- // No FS cleanup; still remove fileset write-through secrets when
cascade deleted them.
- // Schema secrets are cleaned by SchemaOperationDispatcher.
- if (dropped && cascade) {
- for (FilesetEntity fileset : filesets) {
- secretManager.deleteSecretsFromProperties(fileset.properties());
- }
+ // Catalog force-drop reaches this method without
SchemaOperationDispatcher's fileset
+ // cleanup. Drop each fileset before its schema so a storage failure
preserves the failed
+ // fileset and its parent for retry. Stop at the first failure; earlier
drops stay committed.
+ if (cascade) {
+ for (FilesetEntity fileset : filesets) {
+ dropFilesetAndGet(fileset.nameIdentifier())
+ .ifPresent(
+ deleted ->
secretManager.deleteSecretsFromProperties(deleted.properties()));
+ }
+
+ // Functions can also live in fileset catalogs. Drop their metadata
explicitly before
+ // the non-cascading schema delete so it still protects any
unprocessed filesets.
+ for (FunctionEntity function :
+ store.list(filesetNs, FunctionEntity.class,
Entity.EntityType.FUNCTION)) {
Review Comment:
nit: The new function `store.list` / `store.delete` calls throw
`IOException` into the existing catch (lines 973-974), which always wraps it as
`Failed to delete schema <ident> location`. A function-metadata failure is then
indistinguishable from directory cleanup. Please wrap those store calls at the
function loop. The `NoSuchEntityException` catch has the same shape: after some
filesets are already deleted, a missing parent during the function list returns
false ("schema does not exist") instead of failing the drop.
--
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]
