github-actions[bot] commented on code in PR #64695:
URL: https://github.com/apache/doris/pull/64695#discussion_r3555926373
##########
fe/fe-filesystem/fe-filesystem-spi/src/main/java/org/apache/doris/filesystem/spi/ObjectStorageUri.java:
##########
@@ -17,21 +17,28 @@
package org.apache.doris.filesystem.spi;
+import java.util.Set;
+
/**
- * Parsed object-storage URI. Extracts bucket and key without any fe-core
dependency.
+ * Parsed object-storage URI (bucket + key). Has no fe-core dependency.
+ *
+ * <p>This is the single shared parser for every S3-compatible provider (S3,
OSS, COS, OBS).
+ * The bucket/key extraction mechanics are genuinely uniform across them; the
per-provider
+ * variation is supplied by the caller: the {@code pathStyleAccess} flag and
the set of accepted
+ * schemes. The provider itself is selected from connection properties
(endpoint domain / explicit
+ * storage type), not from the URI scheme, but each provider still constrains
which schemes it will
+ * accept via {@link #parse(String, boolean, Set)} — so COS accepts {@code
s3}/{@code s3a}/{@code
+ * cos}/{@code cosn} but rejects {@code oss}. Each provider owns its
accepted-scheme set on its
+ * {@code S3CompatibleFileSystemProperties}; this class only enforces it.
*
- * <p>FIXME: this should be an interface, with each provider implementing its
own
- * {@code normalize}. The current single shared parser is essentially the
legacy approach:
- * historically OBS/COS/OSS/S3 URIs all had to be normalized into S3 form,
either to hand
- * to the BE or for the FE's own use. With per-provider native SDKs that
constraint no
- * longer holds. {@code pathStyleAccess} is part of the same legacy and should
not appear
- * here either — it is an S3-family endpoint concern that belongs in the
provider
- * implementations, not in generic URI parsing.
+ * <p>{@code pathStyleAccess} stays here because parsing a path-style HTTP(S)
endpoint URL
+ * genuinely needs it to locate the bucket segment; it is independent of
whether a provider's
+ * client performs path-style <em>addressing</em> (e.g. COS parses path-style
URLs but its SDK
+ * always issues virtual-hosted requests).
*
- * <p>FIXME: the input is a plain {@code String} that is then split on {@code
'/'} below,
- * so URI escaping must already have been applied before a value can be used
as input here
- * (see the note on {@link #parse}). An interface-based design should take a
properly
- * typed/encoded URI instead of a raw string.
Review Comment:
This scheme check breaks the legacy mixed-scheme fallback that
`LocationPath` still implements. For example, if a catalog has only an
S3-compatible storage config but a path comes in as `cos://bucket/key`,
`LocationPath.findStorageProperties` documents that it should fall back to an
available storage whose name is `s3`, and
`S3PropertyUtils.validateAndNormalizeUri` can normalize compatible schemes to
`s3://...`. However `SpiSwitchingFileSystem` only uses that result to
pick/cache the filesystem; the actual operation still passes the original
`Location` URI to the selected S3 filesystem. Before this change the parser
accepted that URI and extracted the same bucket/key. Now the S3 filesystem
passes `Set.of("s3", "s3a", "s3n")` here, so line 99 throws `Unsupported scheme
'cos'` before the request reaches the client. The same applies to the
documented `oss://`/`obs://` compatibility cases. Please either keep this
parser lenient for delegated S3-compatible filesystems or make the fallb
ack rewrite the `Location` URI before the filesystem sees it.
--
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]
