jerryshao commented on code in PR #13506:
URL: https://github.com/apache/gravitino/pull/13506#discussion_r4355637283
##########
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.listStatus(formalizePath).length > 0) {
+ 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");
+ }
}
+ }
+ for (Map.Entry<String, Path> entry :
filesetPathsBuilder.build().entrySet()) {
Review Comment:
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.
##########
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.listStatus(formalizePath).length > 0) {
Review Comment:
`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).
##########
api/src/main/java/org/apache/gravitino/file/Fileset.java:
##########
@@ -71,6 +71,13 @@ public interface Fileset extends Auditable {
/** The property name for the default location name of the fileset. */
String PROPERTY_DEFAULT_LOCATION_NAME = "default-location-name";
+ /**
+ * The property name that allows creating a managed fileset on a location
that already exists and
+ * is not empty. Defaults to false, in which case such a location must be
registered as an
+ * external fileset instead.
+ */
+ String PROPERTY_ALLOW_EXISTING_LOCATION_AS_MANAGED =
"allow-existing-location-as-managed";
+
Review Comment:
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.
##########
docs/filesets.md:
##########
@@ -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
Review Comment:
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.
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -1047,6 +1048,219 @@ public void testCreateFilesetWithExceptions() throws
IOException {
}
}
+ /** Checks that managed filesets reject existing data without registering
metadata. */
Review Comment:
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.
--
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]
