[#12550] improvement(fileset): Guard managed fileset creation on non-empty locations by Ystk-hsn · Pull Request #13506 · apache/gravitino

Ystk-hsn · 2026-09-25T05:20:18Z

What changes were proposed in this pull request?

Reject creation of a MANAGED fileset on an existing non-empty location unless allow-existing-location-as-managed=true is supplied. The property defaults to false, is immutable, and has Java and Python constants.

Preflight all resolved locations before creating directories, preserving nested locations within one request. Check directory contents through listStatusIterator().hasNext() and document the explicit adoption behavior.

Why are the changes needed?

Silently adopting existing data as managed makes that data subject to deletion when the fileset is dropped. This adds an explicit choice at creation time.

Addresses the creation-time protection in #12550. Type conversion is separate work in #14124.

Does this PR introduce any user-facing change?

Yes. Existing non-empty locations require EXTERNAL or an explicit managed-adoption override. Existing empty directories, external filesets, and catalogs with filesystem operations disabled retain their existing behavior.

Drop and cascade behavior are unchanged. Validation is a creation-time preflight; it does not reserve paths against concurrent filesystem writers.

How was this patch tested?

./gradlew :api:build :catalogs:catalog-fileset:build -PskipITs --max-workers=2 --console=plain
Added coverage for inherited and placeholder locations, case-insensitive true versus false/invalid overrides, immutable-property validation, and iterator-based emptiness checks. Existing nested-location and multi-location preflight regression tests remain covered.
Java module builds passed: 300 API tests and 277 fileset tests, with no failures or skips.
Python fileset API unit test, constant import check, Black formatting check, and Pylint passed.

Object-store connector behavior was inspected in the pinned dependency sources; live cloud-storage tests were not run.

…mpty locations and add delete-data-on-drop Creating a MANAGED fileset on a location that already exists and is not empty is rejected unless allow-existing-location-as-managed=true is set; empty existing directories are still adopted. A new mutable fileset property delete-data-on-drop (default true) lets a managed fileset keep its storage when it or its schema is dropped. Both properties are documented. Signed-off-by: Yasutaka Hisano <yasutennis713@gmail.com>

Ystk-hsn · 2026-09-30T15:25:06Z

@yuqi1129 @diqiu50 Could you take a look at this PR when you have a moment? Thanks!

mchades

I reproduced two issues on b8bb3ac.

mchades · 2026-10-21T16:32:52Z

      ClassLoader cl = Thread.currentThread().getContextClassLoader();
      filesets.parallelStream()
-          .filter(f -> f.filesetType() == Fileset.Type.MANAGED)
+          .filter(this::deletesStorageOnDrop)


[P1] Honor committed retention changes during cascade

filesets is read before super.dropSchema(...), so this filter uses a stale snapshot. In a multi-server deployment, another server can successfully set delete-data-on-drop=false after that read but before the metadata deletion; the cascade still sees the old value and deletes the data. I reproduced this interleaving, and forced catalog drops also use this path. Please evaluate retention against the version actually being deleted, as dropFileset does, or abort if it changed.

mchades · 2026-10-21T16:32:52Z

          } else {
+            // A managed fileset takes its data with it on drop, so silently adopting a location
+            // that already holds files would put data Gravitino never created at risk.
+            if (type == Fileset.Type.MANAGED


[P2] Validate every location before creating directories

This check runs in the same loop as mkdirs. With fresh locations a=/new/root/child and b=/new/root, processing a first creates b as a non-empty parent, so the request is rejected even though neither location existed before the call. I reproduced this on the PR head; the pre-change implementation accepts it. When a later location genuinely contains existing data, earlier newly created directories are also left behind after rejection. Please preflight all locations before creating any of them.

mchades · 2026-10-22T03:54:36Z

@Ystk-hsn Please address the two review comments and update the PR.

mchades · 2026-10-23T02:05:25Z

+   * The property name that controls whether dropping a managed fileset also deletes its storage
+   * locations. Defaults to true; set it to false to keep the data while removing the metadata.
+   */
+  String PROPERTY_DELETE_DATA_ON_DROP = "delete-data-on-drop";


@Ystk-hsn With type conversion being added in #14124, I suggest narrowing this PR to the creation guard:

Keep the rejection of Managed filesets over existing non-empty locations, with allow-existing-location-as-managed=true for explicit adoption.

Remove delete-data-on-drop and its associated deletion logic, tests, and documentation. Converting MANAGED to EXTERNAL before dropping will provide the explicit way to retain data, keeping deletion behavior tied to the fileset type.

Please also address the multi-location preflight comment: validate all locations before creating any directories. Removing the retention property would also remove the new behavior discussed in the cascade race comment, so that issue would no longer need to be addressed within this PR.

…ntion Validate all location filesystem states before creating directories. Preserve nested new locations and avoid leaving new directories when a later location is rejected. Remove delete-data-on-drop in favor of the separate type-conversion work in apache#14124. Retain the creation guard and explicit adoption option, with regression coverage and updated documentation.

jerryshao

Thanks @Ystk-hsn for working on this. The create-time guard looks right for the common case: non-empty managed locations are rejected with IllegalArgumentException, empty directories are still adopted, external filesets are unchanged, and disable-filesystem-ops skips both the check and later deletes. The earlier feedback (preflight before any mkdirs, dropping delete-data-on-drop) is addressed in 56642e32e.

A few things to address before merging (details inline):

The adopt loop can still take a directory that became non-empty after the preflight.
listStatus().length > 0 lists every child of exactly the large directories this guard is meant to reject; please use listStatusIterator and confirm object-store semantics.
The docs (filesets.md quick start, manage-fileset-metadata-using-gravitino.md:23) and the PR description still describe the old contract / the removed delete-data-on-drop property.
Missing tests for derived (schema/catalog/placeholder) locations and for the immutability of the new property.

Question on scope vs #14124: this PR only guards create (createMultipleLocationFileset). #14124 can convert an EXTERNAL fileset to MANAGED via alter with no non-empty check, and alter is allowed for WRITE_FILESET while drop is owner-only, so a later owner drop or schema cascade would delete that data. Already-created managed filesets on existing data are also still deleted on drop, so item 2 of #12550 now depends entirely on #14124. If that conversion is the supported opt-in, could we document it in both PRs and have #14124 run this same check (or an equally explicit confirmation) on EXTERNAL -> MANAGED? #14120 does not touch this create path, so there should be no conflict there.

jerryshao · 2026-10-23T02:53:29Z

          }
+        }

+        for (Map.Entry<String, Path> entry : filesetPathsBuilder.build().entrySet()) {


The preflight above is not what gets stored. In this second loop, for an existing path we go to the else branch (line 614, "manages the existing location") without listing it again, and mkdirs also succeeds when the directory already exists. A path that was absent or empty during the first loop can hold data by the time we get here (another writer, or mkdirs of a nested location in this same request), and it will then be stored as MANAGED and recursively deleted on drop. Please treat "absent" / "empty" as a snapshot: on the adopt path, reject the request if the directory now has children that are not other locations from this call, and clean up directories this call already created. Note that a blanket re-check here would break testCreateManagedFilesetWithNestedNewLocations when the child is created first.

This guard is a preflight check. The base exists/mkdirs/metadata-write sequence already permits concurrent filesystem changes, and another check would still leave a race before metadata insertion. The two-pass validation preserves nested locations created by the same request. Concurrency control and cleanup of partially created directories are separate lifecycle changes, so they are outside this PR.

jerryshao · 2026-10-23T02:53:29Z

+            // that already holds files would put data Gravitino never created at risk.
+            if (type == Fileset.Type.MANAGED
+                && !allowExistingLocationAsManaged
+                && fs.listStatus(formalizePath).length > 0) {


listStatus().length > 0 materializes every child just to decide the directory is non-empty, and the directories this guard is meant to reject are exactly the large ones that can make this time out or OOM the server. Also, a listing failure (including AccessControlException) is wrapped as a generic "Failed to create fileset". Please use listStatusIterator and stop at the first real child, as probeLocation already does. Could you also confirm the behavior on S3/GCS/OSS/ABFS: exists() == false must not hide objects under that prefix, and a 0-byte directory marker alone should not count as content (otherwise pre-created empty prefixes would be rejected).

Switched to listStatusIterator(...).hasNext() and covered empty/non-empty results. The IOException wrapper is unchanged from the base.

I checked the pinned Hadoop 3.3.6/GCS 1.9.4 sources: S3A/GCS/OSS filter their own directory marker; ABFS uses the service's path listing. S3A supports incremental listing, ABFS does so when enabled, while GCS/OSS retain the base array-backed iterator. Prefix visibility remains connector/configuration-dependent, including GCS implicit-directory inference. Changing those existing semantics or upgrading connectors is outside this PR. These are source checks, not live cloud tests.

jerryshao · 2026-10-23T02:53:29Z

@@ -55,13 +55,15 @@ See [How to use GVFS](./how-to-use-gvfs.md).
 ### Managed and External

 A managed fileset belongs to Gravitino. Creating it creates the directory, and deleting it deletes


The quick start earlier on this page (line 48) still says that pointing at an existing path makes the fileset external. That is no longer true: the type stays whatever the caller sent, and creating MANAGED on a non-empty location now fails unless allow-existing-location-as-managed=true. The same sentence is in docs/manage-fileset-metadata-using-gravitino.md:23. Please update both, and also rewrite the PR description, since delete-data-on-drop / deletesStorageOnDrop are no longer part of this patch.

Updated the quick start and creation guide to describe the explicit type and adoption override. Rewrote the PR title/description around the creation guard and removed the obsolete retention claims.

jerryshao · 2026-10-23T02:53:29Z

    }
  }

+  /** Checks that managed filesets reject existing data without registering metadata. */


Please add tests for a managed fileset with no explicit storage location whose path is derived from the schema location (and the catalog-location equivalent), and for a {{schema}}/{{fileset}} placeholder path that resolves onto existing files. Also assert that setProperty / removeProperty of allow-existing-location-as-managed is rejected, and that only a boolean true (any case) opts in.

Added coverage for catalog/schema-derived locations and schema/fileset placeholders over existing data, including explicit adoption. Added rejection tests for setting/removing the property through the alter validator, plus true-case variants and false/invalid values.

jerryshao · 2026-10-23T02:53:29Z

+   * external fileset instead.
+   */
+  String PROPERTY_ALLOW_EXISTING_LOCATION_AS_MANAGED = "allow-existing-location-as-managed";
+


nit: please add the matching PROPERTY_ALLOW_EXISTING_LOCATION_AS_MANAGED constant next to the other fileset property constants in the Python client (clients/client-python/gravitino/api/file/fileset.py). Optional: booleanPropertyEntry uses Boolean.valueOf, so yes / 1 are silently treated as false (fail-closed, consistent with other boolean properties); consider rejecting values other than true/false for this property.

Added the Python constant. Keeping the existing boolean decoding semantics: only case-insensitive true opts in; false, yes, 1, and other values fail closed. The new tests cover these cases. Strict parsing would be a separate behavior change.

mchades · 2026-10-23T04:23:49Z

Regarding the scope question:

Type conversion and its authorization belong to #14124. This PR is limited to create-time validation and keeps existing managed-fileset deletion and cascade behavior unchanged. Retaining existing data by converting to EXTERNAL is separate work; the updated PR description identifies this as the creation-time part of #12550.

Ystk-hsn mentioned this pull request Sep 25, 2026

[Improvement] Managed fileset silently adopts existing directories, causing unintended data deletion on drop #12550

Open

mchades reviewed Oct 21, 2026

View reviewed changes

This was referenced Oct 22, 2026

[Improvement] Support changing filesets between managed and external #14123

Open

[#14123] feat(fileset): Support managed and external type conversion #14124

Open

mchades reviewed Oct 23, 2026

View reviewed changes

mchades added 2 commits October 23, 2026 10:33

Merge branch 'main' into fileset-delete-data-on-drop

a736a18

mchades requested a review from jerryshao October 23, 2026 02:44

jerryshao reviewed Oct 23, 2026

View reviewed changes

[apache#12550] fix(fileset): Address creation guard review feedback

a4929f8

mchades changed the title ~~[#12550] improvement(fileset): Protect data of managed filesets on existing locations and on drop~~ [#12550] improvement(fileset): Guard managed fileset creation on non-empty locations Oct 23, 2026

mchades requested a review from jerryshao October 23, 2026 04:24

yuqi1129 mentioned this pull request Oct 23, 2026

[MINOR] fix(ci): keep Fork CI and allowlist statuses visible #14192

Draft

		@@ -55,13 +55,15 @@ See [How to use GVFS](./how-to-use-gvfs.md).
		### Managed and External

		A managed fileset belongs to Gravitino. Creating it creates the directory, and deleting it deletes

Conversation

Ystk-hsn commented Sep 25, 2026 • edited by mchades 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!

Ystk-hsn commented Sep 30, 2026

Uh oh!

mchades 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!

mchades commented Oct 22, 2026

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!

mchades commented Oct 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ystk-hsn commented Sep 25, 2026 •

edited by mchades

Loading