github-actions[bot] commented on code in PR #66004:
URL: https://github.com/apache/doris/pull/66004#discussion_r3649321836
##########
fe/fe-filesystem/fe-filesystem-jfs/src/main/java/org/apache/doris/filesystem/jfs/JfsFileSystemProvider.java:
##########
@@ -54,10 +53,43 @@ public boolean supports(Map<String, String> properties) {
}
@Override
- public FileSystem create(Map<String, String> properties) throws
IOException {
+ public JfsProperties bind(Map<String, String> properties) {
JfsProperties jfsProperties = new JfsProperties(properties);
jfsProperties.initNormalizeAndCheckProps();
- return new DFSFileSystem(jfsProperties.getBackendConfigProperties());
+ return jfsProperties;
+ }
+
+ @Override
+ public FileSystem create(JfsProperties properties) throws IOException {
+ return new DFSFileSystem(properties.getBackendConfigProperties());
Review Comment:
[P1] Preserve Kerberos semantics in the JFS typed binding
`jfs://` used to bind through `HdfsProperties`, which validates both
supported Kerberos alias families, translates them to Hadoop keys, and creates
a Kerberos execution authenticator. The new `JfsProperties` has none of those
auth fields, inherits `isKerberos() == false` and a simple authenticator, and
this direct create receives its unaligned backend map.
`StorageAdapter.alignJfsBackendMap` only repairs facade-derived maps; it cannot
fix this direct path or the Iceberg/Paimon authenticator delegated from the SPI
object. Consequently a JFS catalog using
`hdfs.authentication.kerberos.{principal,keytab}` can initialize without the
legacy validation and then run outside the requested Kerberos identity. Please
reuse the HDFS auth binding/validation/authenticator behavior here and cover
both alias families, missing credentials, direct creation, and metastore
authenticator selection.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/storage/StorageUriUtils.java:
##########
@@ -0,0 +1,370 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.datasource.storage;
+
+import org.apache.doris.common.UserException;
+import org.apache.doris.common.util.S3URI;
+import org.apache.doris.foundation.property.StoragePropertiesException;
+
+import org.apache.commons.lang3.StringUtils;
+
+import java.io.UnsupportedEncodingException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import java.util.Optional;
+import java.util.regex.Pattern;
+
+/**
+ * fe-core-only URI validation/normalization glue for {@link StorageAdapter}.
+ *
+ * <p>Ports the URI subset of the legacy {@code S3PropertyUtils} and {@code
AzurePropertyUtils}
+ * pure functions so the facade does not depend on the to-be-deleted
+ * {@code datasource.property.storage} package. Behavior is kept identical,
with one deliberate
+ * exception: the legacy full-parse path could throw the checked {@code
UserException} (from
+ * {@link S3URI#create}); the facade surface is unchecked, so that exception
is rethrown as
+ * {@link StoragePropertiesException} with the same message.</p>
+ */
+public final class StorageUriUtils {
+
+ private static final String URI_KEY = "uri";
+ private static final String SCHEME_DELIM = "://";
+ private static final String S3_SCHEME_PREFIX = "s3://";
+
+ // S3-compatible schemes that can be converted to s3:// with simple string
replacement
+ // Format: scheme://bucket/key -> s3://bucket/key
+ private static final String[] SIMPLE_S3_COMPATIBLE_SCHEMES = {
+ "s3a", "s3n", "oss", "cos", "cosn", "obs", "bos", "gs"
+ };
+
+ private static final Pattern ONELAKE_PATTERN = Pattern.compile(
+
"abfs[s]?://([^@]+)@([^/]+)\\.dfs\\.fabric\\.microsoft\\.com(/.*)?",
Pattern.CASE_INSENSITIVE);
+
+ private StorageUriUtils() {
+ }
+
+ /**
+ * Port of legacy {@code S3PropertyUtils.validateAndNormalizeUri}:
validates and normalizes
+ * the given path into a standard {@code s3://bucket/key} URI.
+ *
+ * <p>Also carries the legacy {@code
OSSProperties.validateAndNormalizeUri} override: OSS
+ * bindings first rewrote a virtual-hosted {@code
scheme://bucket.endpoint/key} authority to
+ * {@code scheme://bucket/key} ({@code rewriteOssBucketIfNecessary})
before the base
+ * normalization, so an OSS bucket-domain URI under a non-http scheme
never reached BE or the
+ * filesystem with the whole host as the bucket. The facade has no
provider key at this call
+ * site (StorageAdapter routes every S3-compatible binding here), so the
rewrite is gated on
+ * the URI shape instead — see {@link
#rewriteOssBucketIfNecessary(String)}.</p>
+ */
+ public static String validateAndNormalizeS3Uri(String path,
+ String stringUsePathStyle,
+ String stringForceParsingByStandardUri) {
+ if (StringUtils.isBlank(path)) {
+ throw new StoragePropertiesException("path is null");
+ }
+
+ // Legacy OSSProperties applied the bucket-domain rewrite before the
base normalization.
+ path = rewriteOssBucketIfNecessary(path);
Review Comment:
[P1] Apply the OSS bucket-domain rewrite only to OSS bindings
This helper is called for every S3-compatible adapter, so the OSS-specific
rewrite changes non-OSS behavior. With an explicit generic S3 binding
(`fs.s3.support=true`) and `oss://logs.prod/archive/a.parquet`, legacy
`S3Properties` returned `s3://logs.prod/archive/a.parquet`; this code first
truncates the authority and returns `s3://logs/archive/a.parquet`, silently
targeting a different bucket. Please carry provider identity into normalization
(or apply this rewrite only in the OSS adapter) and add parity cases for dotted
`oss://` and Aliyun-looking `s3://` authorities under explicit non-OSS bindings.
##########
fe/fe-filesystem/fe-filesystem-minio/src/main/java/org/apache/doris/filesystem/minio/MinioFileSystemProvider.java:
##########
@@ -65,6 +65,83 @@ public FileSystem create(MinioFileSystemProperties
properties) throws IOExceptio
new S3ObjStorage(delegate, properties.getSupportedSchemes()));
}
+ private static final String[] MINIO_GUESS_IDENTIFIERS = {
+ "minio.access_key", "AWS_ACCESS_KEY", "ACCESS_KEY", "access_key",
"s3.access_key",
+ "minio.endpoint", "s3.endpoint", "AWS_ENDPOINT", "endpoint",
"ENDPOINT"};
+ private static final String[] MINIO_GUESS_ENDPOINT_NAMES = {
+ "minio.endpoint", "s3.endpoint", "AWS_ENDPOINT", "endpoint",
"ENDPOINT"};
+ // Endpoint markers of the clouds whose guessIsMe fe-core's
MinioProperties.guessIsMe
+ // consults for mutual exclusion (Azure/COS/OSS/S3, plus GCS which the
dialect split adds).
+ private static final String[] OTHER_CLOUD_ENDPOINT_MARKERS = {
+ "aliyuncs.com", "myqcloud.com", "amazonaws.com",
"storage.googleapis.com",
+ ".blob.core.windows.net", ".dfs.core.windows.net",
".blob.core.chinacloudapi.cn"};
+ private static final String[] S3_GUESS_REGION_NAMES = {
+ "s3.region", "glue.region", "aws.glue.region",
"iceberg.rest.signing-region",
+ "rest.signing-region", "client.region"};
+
+ @Override
+ public boolean supportsExplicit(Map<String, String> properties) {
+ return S3CompatSignals.isFsSupport(properties, FS_MINIO_SUPPORT);
+ }
+
+ @Override
+ public boolean supportsGuess(Map<String, String> properties) {
+ // Verbatim port of fe-core MinioProperties.guessIsMe: MinIO is the
"any other
+ // S3-compatible" fallback — it claims the map iff none of
Azure/COS/OSS/S3 would claim
+ // it AND at least one identifier key is present. The sibling guesses
are replicated
+ // here (plugins cannot call each other's classes), preserving legacy
control flow
+ // EXACTLY: legacy S3.guessIsMe SHORT-CIRCUITS on a present endpoint
alias (claiming
+ // only an amazonaws endpoint), and consults its uri/region fallbacks
ONLY when no
+ // endpoint alias exists. A present non-cloud endpoint (e.g. a private
MinIO address)
+ // therefore stays MinIO's even when an s3.region key is also set.
+ if
("azure".equalsIgnoreCase(properties.get(S3CompatSignals.PROVIDER_KEY))) {
+ return false;
+ }
+ String endpoint = null;
+ for (String name : MINIO_GUESS_ENDPOINT_NAMES) {
+ String value = properties.get(name);
+ if (value != null && !value.isBlank()) {
+ endpoint = value;
+ break;
+ }
+ }
+ if (endpoint != null) {
+ String lower = endpoint.toLowerCase(java.util.Locale.ROOT);
+ for (String marker : OTHER_CLOUD_ENDPOINT_MARKERS) {
Review Comment:
[P1] Reuse Azure's host-suffix predicate for the MinIO exclusion
This marker scan is not equivalent to the legacy `AzureProperties.guessIsMe`
call. For example, `https://acct.dfs.core.usgovcloudapi.net` is recognized by
the Azure provider from `Config.azure_blob_host_suffixes`, but that suffix is
absent from this marker list, so `bindAll` also creates a MINIO binding.
`CatalogProperty` keeps both, and S3-style location and connectivity fallbacks
prefer MINIO, routing the Azure catalog through the wrong provider. Custom
mutable suffixes have the same problem, while `contains` can also reject a
non-Azure host merely containing a public suffix. Please use the same host
extraction and injected live suffix list as the Azure provider and add full
`bindPrimary`/`bindAll` parity coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/fs/FileSystemPluginManager.java:
##########
@@ -141,6 +158,162 @@ public void registerProvider(FileSystemProvider provider)
{
DatasourcePrintableMap.registerSensitiveKeys(provider.sensitivePropertyKeys());
}
+ /** Returns an unmodifiable view of the loaded providers, in registration
order. */
+ public List<FileSystemProvider> getProviders() {
+ return Collections.unmodifiableList(providers);
+ }
+
+ /**
+ * Selection priority for raw-user-props binding = {@code
StorageRegistry.Provider} declaration
+ * order (single source of truth; mirrors the legacy
StorageProperties.PROVIDERS registry
+ * order, with JFS hoisted ahead of HDFS so a jfs:// uri beats HDFS's
key-hint guess).
+ */
+
+ private static final String DEPRECATED_OSS_HDFS_SUPPORT =
"oss.hdfs.enabled";
+
+ /**
+ * Binds the first matching provider for raw user properties, mirroring
+ * {@code StorageProperties.createPrimary}: fixed priority, explicit
{@code fs.<x>.support}
+ * flags first-class, heuristic guesses globally disabled once any
explicit flag is present,
+ * and no default fallback — throws when nothing matches.
+ */
+ public FileSystemProperties bindPrimary(Map<String, String> properties) {
+ boolean useGuess = !hasAnyExplicitFsSupport(properties);
+ Map<String, String> probeView = withProbeContext(properties);
+ for (StorageRegistry.Provider meta :
StorageRegistry.Provider.values()) {
+ FileSystemProvider provider = providerByName(meta.name());
+ if (provider == null) {
+ continue;
+ }
+ if (provider.supportsExplicit(properties) || (useGuess &&
provider.supportsGuess(probeView))) {
+ return provider.bind(properties);
+ }
+ }
+ // Providers not in the StorageRegistry.Provider table (out-of-tree
plugins) are consulted
+ // after every known provider, in registration order — adding a plugin
needs no fe-core
+ // priority-list change and can never preempt the legacy fourteen.
+ for (FileSystemProvider provider : providers) {
+ if (StorageRegistry.Provider.byName(provider.name()).isPresent()) {
+ continue;
+ }
+ if (provider.supportsExplicit(properties) || (useGuess &&
provider.supportsGuess(probeView))) {
Review Comment:
[P1] Keep legacy out-of-tree providers selectable
A provider compiled against the documented SPI implements `supports(Map)`
and `create(Map)` only. It therefore inherits `supportsExplicit() == false`,
`supportsGuess() == false`, and the throwing default `bind()`, so this new loop
never even consults its `supports()` result. All migrated storage consumers
bind through `StorageAdapter` first, which makes such directory-loaded
providers invisible before the existing raw-map creation fallback can run. This
also contradicts `FileSystemProvider.bind`'s statement that legacy providers
may keep implementing `create(Map)` during migration; the extensibility test
covers only a newly typed provider. Please retain a raw-properties bridge for
legacy providers (including `supports()` selection and a bind representation),
or explicitly version/reject the old SPI instead of silently skipping 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]
