github-actions[bot] commented on code in PR #64695:
URL: https://github.com/apache/doris/pull/64695#discussion_r3592313372
##########
fe/fe-filesystem/fe-filesystem-hdfs/src/main/java/org/apache/doris/filesystem/hdfs/HdfsFileSystemProvider.java:
##########
@@ -47,17 +56,29 @@ public boolean supports(Map<String, String> properties) {
if (uri != null) {
int schemeEnd = uri.indexOf("://");
if (schemeEnd > 0) {
- String scheme = uri.substring(0, schemeEnd).toLowerCase();
- return SUPPORTED_SCHEMES.contains(scheme);
+ // A concrete scheme is authoritative: only hdfs/viewfs are
ours. jfs:// and oss://
+ // are served by sibling filesystem plugins and ofs:// is
broker-routed by fe-core,
+ // so all are declined here even though fe-core also marks jfs
as "HDFS".
+ return SUPPORTED_SCHEMES.contains(uri.substring(0,
schemeEnd).toLowerCase(Locale.ROOT));
}
}
+ // No uri scheme: fall back to the authoritative HDFS marker or
HA/kerberos hints
+ // (e.g. Hive catalog properties without explicit connection URIs).
+ if ("HDFS".equals(storageType)) {
Review Comment:
[P1] Mirror explicit provider selection in raw-map HDFS routing.
`StorageProperties.createPrimary` treats `fs.hdfs.support=true` as
authoritative and disables all guessing whenever any `fs.*.support=true` flag
is present, but this predicate does neither. Consequently
`{fs.hdfs.support=true, hadoop.config.resources=...}` can be rejected before
XML supplies `fs.defaultFS`, while an explicitly OSS-HDFS map that also
contains `dfs.nameservices` is claimed by the alphabetically earlier HDFS
plugin under first-match-wins selection. Please honor the explicit HDFS flag
and suppress these HA/Kerberos heuristics when another provider was explicitly
selected, with a combined-manager test for both shapes.
##########
fe/fe-filesystem/fe-filesystem-api/src/main/java/org/apache/doris/filesystem/properties/S3CompatibleFileSystemProperties.java:
##########
@@ -65,6 +67,16 @@ public interface S3CompatibleFileSystemProperties extends
FileSystemProperties {
/** Returns whether path-style bucket addressing is enabled, as a raw
provider property value. */
String getUsePathStyle();
+ /**
+ * Returns the URI schemes this provider accepts, lower-cased (e.g. {@code
{s3, s3a, oss}}).
+ *
+ * <p>This is the single source of truth for the provider's scheme
identity: URI parsing
+ * rejects any scheme not in this set (so a COS provider refuses {@code
oss://}), and
+ * scheme-to-storage routing can read the same set. Every S3-compatible
provider accepts the
+ * historically normalized {@code s3}/{@code s3a} form in addition to its
native scheme(s).
+ */
+ Set<String> getSupportedSchemes();
Review Comment:
[P1] Keep the host-provided filesystem API binary compatible. Filesystem
plugins deliberately compile API/SPI as `provided`, and
`org.apache.doris.filesystem.*` is loaded parent-first, so a plugin retained
across an FE upgrade executes against these new host classes. Making
`getSupportedSchemes()` abstract while also removing the two-argument
`S3CompatibleFileSystem` constructor makes an existing S3-compatible plugin
fail with `AbstractMethodError` or `NoSuchMethodError`. Please retain a default
method and a deprecated delegating constructor for a compatibility window, and
cover loading a fixture compiled against the previous API.
##########
fe/fe-core/src/main/java/org/apache/doris/fs/SpiSwitchingFileSystem.java:
##########
@@ -114,69 +182,119 @@ public FileSystem forLocation(Location location) throws
IOException {
@Override
public boolean exists(Location location) throws IOException {
- return forLocation(location).exists(location);
+ Resolved r = resolve(location);
+ return r.fs.exists(r.toDelegate(location));
}
@Override
public void mkdirs(Location location) throws IOException {
- forLocation(location).mkdirs(location);
+ Resolved r = resolve(location);
+ r.fs.mkdirs(r.toDelegate(location));
}
@Override
public void delete(Location location, boolean recursive) throws
IOException {
- forLocation(location).delete(location, recursive);
+ Resolved r = resolve(location);
+ r.fs.delete(r.toDelegate(location), recursive);
}
@Override
public void rename(Location src, Location dst) throws IOException {
- forLocation(src).rename(src, dst);
+ Resolved r = resolve(src);
+ r.fs.rename(r.toDelegate(src), r.toDelegate(dst));
Review Comment:
[P1] Normalize both rename operands independently. The source-derived
`Resolved` mapping is also applied to `dst`, so `rename(s3://bucket/tmp,
cos://bucket/final)` resolves the source directly, leaves the destination
unchanged, and now fails the S3 delegate's strict scheme check. Hive
existing-partition insert/alter reaches this shape because BE writes to the
normalized `s3://` path while the target is rebuilt from the raw HMS
compatibility URI. Please resolve both operands, verify that they select the
same delegate/storage, and pass each normalized operand; add
native-source/compatibility-destination tests for `rename` and
`renameDirectory`.
##########
fe/fe-core/src/main/java/org/apache/doris/fs/SpiSwitchingFileSystem.java:
##########
@@ -101,11 +132,48 @@ public FileSystem forPath(String uri) throws IOException {
} catch (UncheckedIOException e) {
throw e.getCause();
}
+ String[] prefixes = compatSchemePrefixes(uri, lp, sp);
+ if (prefixes == null) {
+ return new Resolved(fs, null, null);
+ }
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Legacy scheme fallback for path {}: delegating as
{}...", uri, prefixes[1]);
+ }
+ return new Resolved(fs, prefixes[0], prefixes[1]);
}
- /** Resolves the appropriate {@link FileSystem} for the given {@link
Location}. */
- public FileSystem forLocation(Location location) throws IOException {
- return forPath(location.uri());
+ /**
+ * Returns {@code [callerPrefix, delegatePrefix]} (e.g. {@code ["cos://",
"s3://"]}) when the
+ * legacy cross-scheme fallback fired and the normalization is a pure
scheme-prefix swap, or
+ * {@code null} when no translation must happen. The two conditions:
+ * <ul>
+ * <li>the storage type selected by {@link LocationPath} differs from
the type the path's
+ * scheme maps to — i.e. the compatibility fallback in
+ * {@code LocationPath.findStorageProperties} picked the storage,
not a direct match;</li>
+ * <li>the normalized URI equals the original with only the scheme
prefix replaced, so the
+ * translation is exactly reversible for URIs the delegate
returns.</li>
+ * </ul>
+ */
+ private static String[] compatSchemePrefixes(String uri, LocationPath lp,
StorageProperties sp) {
+ StorageProperties.Type schemeType =
LocationPath.fromSchemaWithContext(uri, lp.getSchema());
+ if (schemeType == sp.getType()) {
Review Comment:
[P1] Cover every concrete-S3 boundary for logical GCS storage. `gs://` maps
directly to `Type.GCS`, but GCS is physically served by the generic S3 provider
(there is no GCS plugin), `GCSProperties` normalizes it to `s3://`, and that
delegate only accepts `s3/s3a/s3n`. This equality therefore skips the
translation and switching operations fail with `Unsupported scheme 'gs'`. A
local switching fix alone is insufficient: Iceberg's supported `DelegateFileIO`
creates the same concrete filesystem and forwards raw `gs://` metadata paths
directly for read/write/delete. Please either make the selected delegate accept
the logical GCS scheme or normalize at both boundaries while preserving
caller-facing metadata, with switching and `DelegateFileIO` tests.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
