mchades commented on code in PR #13506:
URL: https://github.com/apache/gravitino/pull/13506#discussion_r4384903494
##########
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:
Qualified the rejection statements with “when filesystem operations are
enabled.” Catalogs with `disable-filesystem-ops=true` skip this check.
##########
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:
Updated the error message to tell callers to supply the resolved location
explicitly when registering an `EXTERNAL` fileset.
##########
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:
Added a drop assertion verifying that the adopted file is deleted. The three
tests now use per-invocation temporary directories for isolation and cleanup.
--
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]
