jerryshao commented on code in PR #13506:
URL: https://github.com/apache/gravitino/pull/13506#discussion_r4384236595
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetPropertiesMetadata.java:
##########
@@ -73,6 +74,16 @@ protected Map<String, PropertyEntry<?>>
specificPropertyEntries() {
true /* immutable */,
null,
false /* hidden */))
+ .put(
+ PROPERTY_ALLOW_EXISTING_LOCATION_AS_MANAGED,
+ PropertyEntry.booleanPropertyEntry(
+ PROPERTY_ALLOW_EXISTING_LOCATION_AS_MANAGED,
+ "Whether a managed fileset may be created on a non-empty
location that already exists",
+ false /* required */,
+ true /* immutable */,
+ false /* default value */,
+ false /* hidden */,
+ false /* reserved */))
Review Comment:
[Question] The entry is immutable and not scoped to a fileset type, so
`allow-existing-location-as-managed=true` passed when creating an `EXTERNAL`
fileset is accepted, stored permanently, and never read: the only reader is
`FilesetCatalogOperations.java:550`, inside the guard that runs for `MANAGED`
only (`:575-576`). Because the entry is immutable, it cannot be removed
afterwards either, which the new `TestFilesetPropertiesMetadata` confirms.
On its own that is harmless noise, but combined with the unguarded
`updateType` path noted on the create guard it leaves a permanent, silent
adoption flag on filesets where it meant nothing at creation time. Would it be
better to reject the property at create time for `EXTERNAL`, or to document in
the property description that it applies to `MANAGED` creation only and is
otherwise ignored?
Verified by: `grep -rn "allow-existing-location-as-managed" --include=*.java
--include=*.py .` (readers: `FilesetCatalogOperations.java:550` only), plus
reading `:492-495` and `:575-590`.
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -542,25 +543,58 @@ public Fileset createMultipleLocationFileset(
});
} else {
try {
- // formalize the path to avoid path without scheme, uri, authority,
etc.
+ boolean allowExistingLocationAsManaged =
+ (boolean)
+ propertiesMetadata
+ .filesetPropertiesMetadata()
+ .getOrDefault(properties,
PROPERTY_ALLOW_EXISTING_LOCATION_AS_MANAGED);
+ Map<String, Map<String, String>> fileSystemConfigurations =
Maps.newHashMap();
+ // Formalize and validate every location before creating any
directories. Otherwise,
+ // creating a child location can make another location in the request
non-empty.
for (Map.Entry<String, Path> entry : filesetPaths.entrySet()) {
Map<String, String> fsConf =
mergeUpLevelConfigurations(ident, properties, entry.getValue());
+ fileSystemConfigurations.put(entry.getKey(), fsConf);
FileSystem tmpFs = getFileSystemWithCache(entry.getValue(), fsConf);
Path formalizePath =
entry.getValue().makeQualified(tmpFs.getUri(),
tmpFs.getWorkingDirectory());
filesetPathsBuilder.put(entry.getKey(), formalizePath);
FileSystem fs = getFileSystemWithCache(formalizePath, fsConf);
- if (fs.exists(formalizePath) &&
fs.getFileStatus(formalizePath).isFile()) {
- throw new IllegalArgumentException(
- "Fileset location cannot be a file: "
- + formalizePath
- + ", location name: "
- + entry.getKey());
+ if (fs.exists(formalizePath)) {
+ if (fs.getFileStatus(formalizePath).isFile()) {
+ throw new IllegalArgumentException(
+ "Fileset location cannot be a file: "
+ + formalizePath
+ + ", location name: "
+ + entry.getKey());
+ }
+ // 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
+ && !allowExistingLocationAsManaged
Review Comment:
[Question] This guard can now be bypassed from inside this branch, because
the type conversion it defers to has landed in the base.
`FilesetChange.updateType` / `UpdateFilesetType` are present on
`origin/main`
(`api/src/main/java/org/apache/gravitino/file/FilesetChange.java:66` and
`:236`, at `origin/main` = `0562a904`), so they came in with the `256bd24f`
merge. `updateFilesetEntity` applies the new type with no inspection of the
storage location (`FilesetCatalogOperations.java:1281-1283`), `alterFileset`
adds no location check of its own, and `dropFileset` deletes storage for any
fileset whose stored type is `MANAGED` (`:1084`). So `create EXTERNAL on
existing non-empty data` followed by `updateType(MANAGED)` reaches exactly the
state this guard rejects, with no override property involved.
When the scope question was answered on 2026-10-23 this was still future
work in #14124. After the merge it is in the tree, so the creation guard is the
only check that exists. Two reasonable options: have the conversion run this
same emptiness check (or require `allow-existing-location-as-managed`) on
`EXTERNAL -> MANAGED`, or state explicitly in `docs/filesets.md` and in the new
property description that conversion is an unguarded opt-in, so readers do not
take the create-time guard for a general protection of pre-existing data.
Verified by: reading `FilesetChange.java` at `origin/main`, and
`FilesetCatalogOperations.java:1268-1287` and `:1084` at this head; `grep -rn
allow-existing-location-as-managed --include=*.java .` shows the new property
is read only in the create path (`:550`).
##########
docs/fileset-catalog.md:
##########
@@ -206,6 +206,7 @@ Besides, the Fileset catalog fileset has the following
properties:
| `credential-providers` | The credential provider types,
separated by comma.
| (none)
| No
| No |
| `placeholder-` | Properties that start with
`placeholder-` are used to replace placeholders in the location.
| (none)
| No
| Yes |
| `default-location-name` | The name of the default location of
the fileset, mainly used for GVFS operations without specifying a location
name. | When the fileset has only one location, its location name will be
automatically selected as the default value. | Yes, if the fileset has multiple
locations | Yes |
+| `allow-existing-location-as-managed` | Whether a managed fileset may be
created on a location that already exists and is not empty. Such a location is
rejected by default; register it as an `EXTERNAL` fileset instead. | false
| No
| Yes |
Review Comment:
[Nit] This row, and `docs/filesets.md:61-64` and
`docs/manage-fileset-metadata-using-gravitino.md:24-26`, state the rejection
unconditionally. It does not apply to a catalog with
`disable-filesystem-ops=true`: that path returns through the early branch at
`FilesetCatalogOperations.java:527-542` and never reaches the guard, so a
`MANAGED` fileset is created over existing data with no override and no error.
The PR's own `testCreateManagedFilesetOnNonEmptyLocationWhenFSOpsDisabled`
asserts precisely that. Since the PR description already calls this out, one
clause in the docs (`rejected by default when filesystem operations are
enabled`) would keep the user-facing text accurate.
Verified by: reading `FilesetCatalogOperations.java:527-542` (the
`disableFSOps` branch) and the test at
`TestFilesetCatalogOperations.java:1832-1852`.
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -1508,6 +1510,346 @@ public void testCreateFilesetWithExceptions() throws
IOException {
}
}
+ /** Checks that managed filesets reject existing data without registering
metadata. */
+ @Test
+ public void testCreateManagedFilesetRejectsNonEmptyExistingLocation() throws
IOException {
+ String schemaName = "schema_managed_non_empty";
+ String filesetName = "fileset_managed_non_empty";
+ String catalogPath = TEST_ROOT_PATH + "/catalog_managed_non_empty";
+ createSchema(schemaName, "comment", catalogPath, null);
+
+ File existingLocation = new File(UNFORMALIZED_TEST_ROOT_PATH,
"existing_reports");
+ FileUtils.forceMkdir(existingLocation);
+ FileUtils.touch(new File(existingLocation, "report.csv"));
+ NameIdentifier filesetIdent = NameIdentifier.of("m1", "c1", schemaName,
filesetName);
+
+ IllegalArgumentException exception =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ createFileset(
+ filesetName,
+ schemaName,
+ "comment",
+ Fileset.Type.MANAGED,
+ catalogPath,
+ existingLocation.toURI().toString()));
+ Assertions.assertTrue(exception.getMessage().contains("already exists and
is not empty"));
+
Assertions.assertTrue(exception.getMessage().contains(Fileset.Type.EXTERNAL.name()));
+
+ // Nothing was registered, and the data that would have been at risk is
untouched.
+ try (FilesetCatalogOperations ops = new FilesetCatalogOperations(store,
secretManager)) {
+ ops.initialize(
+ ImmutableMap.of(LOCATION, catalogPath),
+ randomCatalogInfo("m1", "c1"),
+ FILESET_PROPERTIES_METADATA);
+ Assertions.assertThrows(NoSuchFilesetException.class, () ->
ops.loadFileset(filesetIdent));
+ }
+ Assertions.assertTrue(new File(existingLocation, "report.csv").exists());
+
+ FileUtils.deleteDirectory(existingLocation);
+ }
+
+ /** Checks that an empty existing directory can still be adopted by a
managed fileset. */
+ @Test
+ public void testCreateManagedFilesetAdoptsEmptyExistingLocation() throws
IOException {
+ String schemaName = "schema_managed_empty";
+ String filesetName = "fileset_managed_empty";
+ String catalogPath = TEST_ROOT_PATH + "/catalog_managed_empty";
+ createSchema(schemaName, "comment", catalogPath, null);
+
+ File existingLocation = new File(UNFORMALIZED_TEST_ROOT_PATH,
"existing_empty");
+ FileUtils.forceMkdir(existingLocation);
+
+ Fileset fileset =
+ createFileset(
+ filesetName,
+ schemaName,
+ "comment",
+ Fileset.Type.MANAGED,
+ catalogPath,
+ existingLocation.toURI().toString());
+ Assertions.assertEquals(Fileset.Type.MANAGED, fileset.type());
+
+ FileUtils.deleteDirectory(existingLocation);
+ }
+
+ /** Checks that explicitly allowing adoption permits a managed fileset over
existing data. */
+ @ParameterizedTest
+ @ValueSource(strings = {"true", "TRUE", "TrUe"})
+ public void testCreateManagedFilesetOnNonEmptyLocationWithOverride(String
value)
+ throws IOException {
+ String schemaName = "schema_managed_override_" + value;
+ String filesetName = "fileset_managed_override";
+ String catalogPath = TEST_ROOT_PATH + "/catalog_managed_override";
+ createSchema(schemaName, "comment", catalogPath, null);
+
+ File existingLocation = new File(UNFORMALIZED_TEST_ROOT_PATH,
"existing_override");
+ FileUtils.forceMkdir(existingLocation);
+ FileUtils.touch(new File(existingLocation, "report.csv"));
+
+ Fileset fileset =
+ createFileset(
+ filesetName,
+ schemaName,
+ "comment",
+ Fileset.Type.MANAGED,
+ catalogPath,
+ existingLocation.toURI().toString(),
+ ImmutableMap.of(PROPERTY_ALLOW_EXISTING_LOCATION_AS_MANAGED,
value));
+ Assertions.assertEquals(Fileset.Type.MANAGED, fileset.type());
Review Comment:
[Nit] This stops at asserting the type. The behavior the override exists to
authorize, that the adopted data is deleted when the fileset is dropped
(`docs/filesets.md:61-64`), is not asserted anywhere: the three usages of the
property in this file are at `:1599`, `:1627` and `:1677`, and the only new
test that calls `dropFileset` is `testCreateExternalFilesetOnNonEmptyLocation`,
which asserts the opposite case. Dropping the adopted fileset here and
asserting `report.csv` is gone would pin the contract both docs and the
property description now promise.
Separately, `existingLocation` is a fixed directory name shared by the three
parameter runs, and `FileUtils.deleteDirectory` only runs on the success path,
so the first failing run leaves `report.csv` behind and the remaining runs then
fail for an unrelated reason, hiding the original one. A `try`/`finally` or
`@TempDir` would make the failure readable. The same pattern is in
`testCreateManagedFilesetRejectsNonEmptyExistingLocation` and
`testCreateManagedFilesetAdoptsEmptyExistingLocation`.
Verified by: `grep -n PROPERTY_ALLOW_EXISTING_LOCATION_AS_MANAGED` over this
file and reading the new tests at `:1513-1852`.
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -542,25 +543,58 @@ public Fileset createMultipleLocationFileset(
});
} else {
try {
- // formalize the path to avoid path without scheme, uri, authority,
etc.
+ boolean allowExistingLocationAsManaged =
+ (boolean)
+ propertiesMetadata
+ .filesetPropertiesMetadata()
+ .getOrDefault(properties,
PROPERTY_ALLOW_EXISTING_LOCATION_AS_MANAGED);
+ Map<String, Map<String, String>> fileSystemConfigurations =
Maps.newHashMap();
+ // Formalize and validate every location before creating any
directories. Otherwise,
+ // creating a child location can make another location in the request
non-empty.
for (Map.Entry<String, Path> entry : filesetPaths.entrySet()) {
Map<String, String> fsConf =
mergeUpLevelConfigurations(ident, properties, entry.getValue());
+ fileSystemConfigurations.put(entry.getKey(), fsConf);
FileSystem tmpFs = getFileSystemWithCache(entry.getValue(), fsConf);
Path formalizePath =
entry.getValue().makeQualified(tmpFs.getUri(),
tmpFs.getWorkingDirectory());
filesetPathsBuilder.put(entry.getKey(), formalizePath);
FileSystem fs = getFileSystemWithCache(formalizePath, fsConf);
- if (fs.exists(formalizePath) &&
fs.getFileStatus(formalizePath).isFile()) {
- throw new IllegalArgumentException(
- "Fileset location cannot be a file: "
- + formalizePath
- + ", location name: "
- + entry.getKey());
+ if (fs.exists(formalizePath)) {
+ if (fs.getFileStatus(formalizePath).isFile()) {
+ throw new IllegalArgumentException(
+ "Fileset location cannot be a file: "
+ + formalizePath
+ + ", location name: "
+ + entry.getKey());
+ }
+ // 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
+ && !allowExistingLocationAsManaged
+ && fs.listStatusIterator(formalizePath).hasNext()) {
+ throw new IllegalArgumentException(
+ "Location "
+ + formalizePath
+ + " for managed fileset "
+ + ident
+ + " already exists and is not empty, location name: "
+ + entry.getKey()
+ + ". Register it as an "
+ + Fileset.Type.EXTERNAL
+ + " fileset, or set "
+ + PROPERTY_ALLOW_EXISTING_LOCATION_AS_MANAGED
+ + "=true to manage it anyway");
Review Comment:
[Nit] The remedy in the message, `Register it as an EXTERNAL fileset`, is
not directly actionable in the derived-location case that
`testCreateManagedFilesetRejectsNonEmptyDerivedLocation` covers: when the
location came from the catalog or schema `location` property and no
`storageLocations` were supplied, creating the fileset as `EXTERNAL` fails with
`Storage location must be set for external fileset` (`:492-495`). The message
already prints the resolved path, so suggesting that it be passed explicitly
(`register it as an EXTERNAL fileset with this location set explicitly`) would
close the loop.
Verified by: reading `FilesetCatalogOperations.java:492-495` and the
derived-location test added in this PR.
--
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]
