mchades commented on code in PR #13506:
URL: https://github.com/apache/gravitino/pull/13506#discussion_r4356065203
##########
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:
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.
##########
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:
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.
##########
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:
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.
--
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]
