[#14119] fix(fileset): Preserve metadata on cascade cleanup failure by mchades · Pull Request #14120 · apache/gravitino

mchades · 2026-10-22T02:17:08Z

What changes were proposed in this pull request?

Delete filesets through the existing single-fileset cleanup path before deleting their schema. Stop on the first storage failure, preserve the failed fileset and parent metadata, and clean each successfully deleted fileset's secrets from its deleted snapshot.

Clean empty schema directories before the schema metadata transaction commits. A schema-specific store operation deletes functions in that same transaction, rejects any remaining fileset, and rolls back schema/function metadata if directory cleanup fails.

Why are the changes needed?

Catalog and metalake force-drop bypass the normal schema dispatcher. Previously, this path removed metadata before cleaning managed fileset storage, leaving failed cleanup without metadata for retry. Schema-directory cleanup could also fail after schema metadata had already committed.

Fix: #14119

Does this PR introduce any user-facing change?

Managed-fileset cleanup failures retain the failed fileset and its parents. Empty schema-directory cleanup failures retain schema and function metadata. Earlier successful fileset deletions remain committed; already removed directories are accepted on retry. EXTERNAL data and data in catalogs with filesystem operations disabled remain untouched.

No REST API or configuration changes.

How was this patch tested?

:catalogs:catalog-fileset:test -PskipITs: 267 tests passed, including schema-directory failure/rollback/retry, function preservation, concurrent fileset creation, and partial multi-location deletion with retry.
:core:coreH2Test --tests '*TestSchemaMetaService' --tests '*TestFilesetMetaService' -PskipITs: 53 tests passed, including schema/function/changelog rollback and error propagation.
:core:coreUnitTest --tests '*TestCatalogManager' --tests '*TestMetalakeManager' --tests '*TestSchemaOperationDispatcher' --tests '*TestFilesetOperationDispatcher' -PskipITs: 84 tests passed.
./gradlew spotlessApply and git diff --check passed.
Docker/MySQL/PostgreSQL and Python integration tests were not rerun for this revision.

…lure

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

3 open issues

super.dropSchema(ident, cascade) can re-introduce the original “cascade deletes metadata without… · New
For the conflict == false branch, the test doesn’t assert anything about the thrown exception… · New
Constructing a fresh Hadoop Configuration per assertion can make unit tests more sensitive to… · New

What changed in this PR

Ensures schema cascade deletes (including catalog/metalake force-drop) preserve metadata when managed-fileset storage cleanup fails, enabling safe retries and cleaning write-through secrets for successfully deleted filesets.

Changes:

Reuse single-fileset deletion flow during dropSchema cascades and stop on first storage cleanup failure.
Clean write-through secrets using each deleted fileset’s snapshot after successful deletion.
Expand unit tests to cover failure/partial-progress/retry behavior and metadata retention expectations.

File	Description
core/src/test/java/org/apache/gravitino/metalake/TestMetalakeManager.java	Adds assertions that force-drop failure retains metalake and catalog metadata.
core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java	Parameterizes force-drop tests to cover schema-op failures and verify metadata retention.
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java	Adds tests for stopping on storage failure, retry semantics, and secret cleanup without storage deletion.
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java	Refactors drop logic to delete filesets before schema metadata and preserve failed state for retry; adds helper `dropFilesetAndGet`.

_{Review effort: Lite}

mchades · 2026-10-22T04:12:03Z

+      // 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()));
        }
-        return dropped;
      }

+      boolean dropped = super.dropSchema(ident, cascade);
      // If the schema entity is failed to be deleted, we should not delete the storage location
      // and return false immediately.
-      if (!dropped) {
-        return false;
+      if (!dropped || disableFSOps) {
+        return dropped;
      }


Changed the final call to super.dropSchema(ident, false) and added a comment explaining the guard. The new regression test creates a fileset after enumeration and verifies its metadata, storage, and secret are preserved until retry.

mchades · 2026-10-22T04:12:18Z

+    RuntimeException thrown =
        Assertions.assertThrows(
-            OptimisticLockException.class, () -> catalogManager.dropCatalog(ident, true)));
+            RuntimeException.class, () -> catalogManager.dropCatalog(ident, true));
+    if (conflict) {
+      Assertions.assertSame(failure, thrown);
+    }
+    Assertions.assertTrue(entityStore.exists(ident, EntityType.CATALOG));
+    Assertions.assertTrue(
+        entityStore.exists(
+            NameIdentifier.of("metalake", ident.name(), "schema_occ"), EntityType.SCHEMA));


Added assertions for the schema context and preservation of the original storage failure in the cause chain.

mchades · 2026-10-22T04:12:29Z

+        Path path = new Path(fileset.storageLocation());
+        Assertions.assertEquals(
+            i != 0, store.exists(fileset.nameIdentifier(), Entity.EntityType.FILESET));
+        Assertions.assertEquals(i != 0, path.getFileSystem(new Configuration()).exists(path));


Both assertion loops now reuse one real local FileSystem, independent of the mocked filesystem used to inject failures.

…sets

jerryshao

Thanks, this looks good overall and fixes #14119 for the force-drop path: a managed storage failure now rolls back that fileset's metadata and aborts the schema drop, secrets are cleaned from the deleted snapshot, and the non-cascading final delete protects filesets created concurrently. None of the comments below are blocking; they are follow-up candidates around schema-directory cleanup, leftover child types, function deletion ordering, secret cleanup on the public cascade path, error wrapping, and test coverage.

jerryshao · 2026-10-22T07:45:35Z

      }

+      // Reject remaining filesets instead of deleting their metadata without storage cleanup.
+      boolean dropped = super.dropSchema(ident, false);


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.

Schema directory cleanup now runs before the metadata transaction commits. Failures retain the schema and its secrets for retry; missing directories are accepted.

jerryshao · 2026-10-22T07:45:35Z

+
+        // 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 :


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.

Tables cannot be created through a fileset catalog's API. Is there a concrete path that produces these stray rows? Otherwise, I'd leave unexpected-metadata recovery outside this PR.

jerryshao · 2026-10-22T07:45:35Z

+                  deleted -> secretManager.deleteSecretsFromProperties(deleted.properties()));
+        }
+
+        // Functions can also live in fileset catalogs. Drop their metadata explicitly before


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.

Function cleanup and schema deletion now share one metadata transaction. The regression for concurrent fileset creation verifies that the function survives a rejected drop and is removed on retry.

jerryshao · 2026-10-22T07:45:35Z

-    } catch (IOException ioe) {
-      throw ExceptionMessages.wrap("Failed to delete fileset " + ident, ioe);
-    }
+    return dropFilesetAndGet(ident).isPresent();


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.

The secret snapshot race in the public deletion path predates this PR. I'm keeping that fix separate from #14119's force-drop change.

jerryshao · 2026-10-22T07:45:35Z

+        // 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)) {


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.

Function cleanup now runs in the schema metadata transaction. Missing-function failures carry function-specific context; other metadata errors retain their exception types instead of being reported as directory cleanup failures or a missing schema.

jerryshao · 2026-10-22T07:45:35Z

+
+  @ParameterizedTest
+  @ValueSource(booleans = {false, true})
+  void testDropSchemaStopsOnStorageFailureAndCanRetry(boolean throwException) throws IOException {


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.

Added cases where the second location deletion returns false or throws IOException. Both verify metadata and secret retention and successful retry with the first location already absent.

jerryshao · 2026-10-22T07:45:35Z

+      // 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) {


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?

Yes. This keeps metadata recoverable on cleanup failure and stops the loop at the first failure. Each fileset has its own transaction, but large trees hold a connection longer. Parallel or asynchronous cleanup should be considered separately.

[apache#14119] fix(fileset): Preserve metadata on cascade cleanup fai…

60cf30f

…lure

Copilot AI balanced review requested due to automatic review settings October 22, 2026 02:17

mchades added the branch-1.3 Automatically cherry-pick commit to branch-1.3 label Oct 22, 2026

mchades requested review from jerryshao and yuqi1129 October 22, 2026 02:20

Copilot AI reviewed Oct 22, 2026

View reviewed changes

[apache#14119] fix(fileset): Guard schema drop against remaining file…

f57867e

…sets

Copilot stopped reviewing on behalf of mchades due to an error October 22, 2026 04:17
Workflow completed but conclusion is not available. This may indicate rate limiting or API issues.

[apache#14119] fix(fileset): Preserve function cleanup on schema cascade

c3cec87

jerryshao reviewed Oct 22, 2026

View reviewed changes

mchades added 2 commits October 22, 2026 18:03

[apache#14119] fix(fileset): Preserve schema metadata on cleanup failure

8aa4092

Merge branch 'main' into codex/fileset-cascade-cleanup-failure

d8a2cdf

mchades requested a review from jerryshao October 22, 2026 14:28

jerryshao approved these changes Oct 23, 2026

View reviewed changes

Merge branch 'main' into codex/fileset-cascade-cleanup-failure

9efb5e5

jerryshao mentioned this pull request Oct 23, 2026

[#12550] improvement(fileset): Guard managed fileset creation on non-empty locations #13506

Open

jerryshao added 2 commits October 23, 2026 14:16

Merge branch 'main' into codex/fileset-cascade-cleanup-failure

a041deb

Merge branch 'main' into codex/fileset-cascade-cleanup-failure

96f90be

Conversation

mchades commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerryshao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mchades commented Oct 22, 2026 •

edited

Loading