mchades commented on code in PR #12553:
URL: https://github.com/apache/gravitino/pull/12553#discussion_r3835691085
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -1065,31 +1101,6 @@ private Map<String, Path>
getAndCheckCatalogStorageLocations(Map<String, String>
checkPlaceholderValue(v);
- if (!disableFSOps && !containsPlaceholder(v)) {
- Path path = new Path(v);
- // At catalog initialization, only merge catalog config and
user-defined location
- // configs
- Map<String, String> fsConf = new HashMap<>(conf);
- fsConf.putAll(
- FilesetUtil.getUserDefinedFileSystemConfigs(
- path.toUri(),
- conf,
-
FilesetCatalogPropertiesMetadata.FS_GRAVITINO_PATH_CONFIG_PREFIX));
- FileSystem fs = getFileSystemWithCache(path, fsConf);
- try {
- if (fs.exists(path) && fs.getFileStatus(path).isFile()) {
- throw new IllegalArgumentException(
- "Fileset catalog location cannot be a file: "
- + v
- + ", location name: "
- + locationName);
- }
- } catch (IOException e) {
- throw new RuntimeException(
- "Failed to check if fileset catalog location exists: " +
v, e);
- }
- }
-
catalogStorageLocations.put(locationName, new
Path((ensureTrailingSlash(v))));
Review Comment:
Thanks for pointing this out. Moving remote filesystem validation out of
catalog initialization is intentional; `testConnection` owns that validation so
catalog loading does not perform remote I/O or credential vending. The probe
already rejects locations that resolve to files. In 0aca32bcb9, I added
coverage for this case and changed the sanitized failure category from `probe
failed` to `location is not a directory`.
--
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]
