github-actions[bot] commented on code in PR #64695: URL: https://github.com/apache/doris/pull/64695#discussion_r3577077975
########## fe/fe-filesystem/fe-filesystem-oss-hdfs/src/main/java/org/apache/doris/filesystem/hdfs/properties/OssHdfsProperties.java: ########## @@ -0,0 +1,167 @@ +// 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.filesystem.hdfs.properties; + +import org.apache.doris.foundation.property.ConnectorProperty; + +import com.google.common.collect.ImmutableSet; +import org.apache.commons.lang3.StringUtils; + +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * Aliyun OSS-HDFS (JindoFS) storage properties for fe-filesystem. + * + * <p>OSS-HDFS is an HDFS-compatible service over OSS, addressed with {@code oss://} URIs and + * served by the JindoFS Hadoop FileSystem ({@code com.aliyun.jindodata.oss.JindoOssFileSystem}). + * It therefore sits on the shared {@link HdfsCompatibleProperties} base alongside plain + * {@link HdfsProperties} and produces a backend config map for {@code DFSFileSystem}; the only + * delta is the Jindo {@code fs.oss.*} wiring plus oss-dls endpoint/region handling.</p> + * + * <p>Self-contained port of the kernel {@code OSSHdfsProperties}, with zero fe-core / fe-common + * dependency. The Jindo impl class names are kept as string constants (loaded reflectively by the + * backend), so no compile dependency on the Jindo SDK is introduced.</p> + */ +public class OssHdfsProperties extends HdfsCompatibleProperties { + + private static final String JINDO_OSS_FILE_SYSTEM_IMPL = + "com.aliyun.jindodata.oss.JindoOssFileSystem"; + private static final String JINDO_OSS_ABSTRACT_FILE_SYSTEM_IMPL = + "com.aliyun.jindodata.oss.JindoOSS"; + + private static final String OSS_HDFS_PREFIX_KEY = "oss.hdfs."; + private static final String OSS_HDFS_ENDPOINT_SUFFIX = ".oss-dls.aliyuncs.com"; + + private static final Set<String> OSS_ENDPOINT_KEY_NAME = ImmutableSet.of("oss.hdfs.endpoint", + "oss.endpoint", "dlf.endpoint", "dlf.catalog.endpoint"); + + private static final Set<Pattern> ENDPOINT_PATTERN = ImmutableSet.of( + Pattern.compile("(?:https?://)?([a-z]{2}-[a-z0-9-]+)\\.oss-dls\\.aliyuncs\\.com"), + Pattern.compile("^(?:https?://)?dlf(?:-vpc)?\\.([a-z0-9-]+)\\.aliyuncs\\.com(?:/.*)?$")); + + private static final Set<String> SUPPORT_SCHEMA = ImmutableSet.of("oss", "hdfs"); + + @ConnectorProperty(names = {"oss.hdfs.endpoint", "oss.endpoint", "dlf.endpoint", "dlf.catalog.endpoint"}, Review Comment: The converted FE path is going to hit this plugin with normalized backend keys, not the raw `oss.hdfs.*` names this binder accepts. `StoragePropertiesConverter.toMap()` copies `props.getBackendConfigProperties()` and then adds `_STORAGE_TYPE_=OSS_HDFS`; for FE `OSSHdfsProperties` that backend map contains `fs.oss.endpoint`, `fs.oss.accessKeyId`, `fs.oss.accessKeySecret`, and `fs.oss.region`. `OssHdfsFileSystemProvider.supports()` selects this provider from the marker, but `create()` rebinds the map here, where none of those `fs.oss.*` keys are listed as aliases, so `endpoint` stays empty and region extraction throws before any OSS-HDFS filesystem can be created. Please either preserve/pass the original OSS-HDFS properties into the plugin or make this plugin-side binder accept the normalized `fs.oss.*` keys emitted by the converter, and add a provider-level test that creates OSS-HDFS from `StoragePropertiesConverter.toMap(...)`. ########## fe/fe-filesystem/fe-filesystem-s3/src/main/java/org/apache/doris/filesystem/s3/S3FileSystemProperties.java: ########## @@ -213,6 +214,11 @@ public void validate() { .validate("Invalid S3 filesystem properties"); } + @Override + public Set<String> getSupportedSchemes() { Review Comment: This whitelist also affects direct concrete S3 filesystems, not just routed catalog paths. For S3 TVF/OUTFILE paths, `ExternalFileTableValuedFunction` creates the concrete provider with `FileSystemFactory.getFileSystem(brokerDesc)` and passes the user URI directly to `globListWithLimit`; it never goes through `SpiSwitchingFileSystem`, so the new switching-boundary rewrite cannot normalize `cos://`/`cosn://`/`obs://`/`oss://`. Existing regression cases use those schemes with `s3.*` properties. With this set limited to `s3/s3a/s3n`, `ObjectStorageUri.parse(..., supportedSchemes)` rejects them before bucket/key parsing. Please keep the direct S3-compatible compatibility surface, or normalize these direct callers before they reach `S3ObjStorage`, and add a provider/TVF test where `supportedSchemes` is non-null. ########## fe/fe-filesystem/fe-filesystem-oss/src/main/java/org/apache/doris/filesystem/oss/OssObjStorage.java: ########## @@ -473,6 +480,51 @@ private static String requireProperty(String value, String key, String descripti return value; } + /** + * Computes the marker that resumes listing on the next page. + * + * <p>OSS (like the S3 V1 {@code GetBucket} API) only fills + * {@link ObjectListing#getNextMarker()} when a {@code delimiter} is set. For a delimiter-less + * (recursive) listing the marker is {@code null} even when {@link ObjectListing#isTruncated()} + * is {@code true}, so naively forwarding it would stop pagination after the first page (~1000 + * keys) and silently drop every object beyond it (recursive delete/rename/list). + * + * <p>When the SDK leaves the marker blank on a truncated page, fall back to the + * lexicographically greatest key/common-prefix returned on this page — OSS lists in + * lexicographic order and resumes strictly after the marker. This mirrors the last-key + * fallback the COS/OBS SDKs perform internally. + * + * <p>If a page is marked truncated yet yields neither a marker nor any key/common-prefix to + * resume from, there is no cursor for the next page; returning {@code null} would make the + * paginating caller re-list from the start indefinitely, so this fails loudly instead. + */ + private static String resolveNextMarker(ObjectListing listing) throws IOException { Review Comment: Returning a fallback marker here fixes provider pagination, but one caller still stops if a truncated page contains only keys filtered by `S3CompatibleFileSystem.ObjectStorageFileIterator`. That iterator fetches one page, drops every key ending in `/`, stores the continuation token, and then `hasNext()` returns false because the buffer is empty; it never loops to the next page while `done=false`. So a truncated OSS page of directory-marker objects followed by real files still makes `list()`/default `listFilesRecursive()` miss later files. Please also make the iterator continue fetching empty filtered pages while a continuation token exists, and add a filesystem-level test for a marker-only truncated page followed by a file. ########## fe/fe-filesystem/fe-filesystem-oss-hdfs/src/main/java/org/apache/doris/filesystem/hdfs/OssHdfsFileSystemProvider.java: ########## @@ -0,0 +1,79 @@ +// 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.filesystem.hdfs; + +import org.apache.doris.filesystem.FileSystem; +import org.apache.doris.filesystem.hdfs.properties.OssHdfsProperties; +import org.apache.doris.filesystem.properties.FileSystemProperties; +import org.apache.doris.filesystem.spi.FileSystemProvider; + +import java.io.IOException; +import java.util.Locale; +import java.util.Map; + +/** + * SPI provider for Aliyun OSS-HDFS (JindoFS), addressed with {@code oss://} URIs and served by an + * HDFS-compatible {@code DFSFileSystem}. Registered via META-INF/services alongside + * {@link HdfsFileSystemProvider}. + * + * <p>Routing is kept strictly disjoint from plain HDFS: the authoritative {@code _STORAGE_TYPE_} + * marker set by fe-core's StoragePropertiesConverter wins, and the heuristic fallback keys on + * {@link OssHdfsProperties#guessIsMe} (oss.hdfs flag / oss-dls endpoint) or an {@code oss://} URI. + * The framework selects providers first-match-wins over an unordered list, so the two providers' + * {@code supports()} predicates must not overlap.</p> + */ +public class OssHdfsFileSystemProvider implements FileSystemProvider<FileSystemProperties> { + + @Override + public boolean supports(Map<String, String> properties) { + String storageType = properties.get("_STORAGE_TYPE_"); + if ("OSS_HDFS".equals(storageType)) { + return true; + } + if ("HDFS".equals(storageType)) { + // An explicit plain-HDFS marker belongs to HdfsFileSystemProvider, never here. + return false; + } + if ("OSS".equals(storageType)) { + // A native-OSS (S3-compatible) marker belongs to OssFileSystemProvider, never here. + return false; + } + // No authoritative marker: fall back to the same detection fe-core uses to pick + // OSSHdfsProperties, plus a bare oss:// URI. + if (OssHdfsProperties.guessIsMe(properties)) { + return true; + } + String uri = properties.get("HDFS_URI"); + if (uri == null) { + uri = properties.get("fs.defaultFS"); + } + return uri != null && uri.toLowerCase(Locale.ROOT).startsWith("oss://"); Review Comment: The raw-map provider predicates still overlap and miss the explicit OSS-HDFS flags. `StorageProperties.createPrimary` treats `fs.oss-hdfs.support=true` and `oss.hdfs.enabled=true` as OSS-HDFS before native OSS, but this provider only recognizes `_STORAGE_TYPE_=OSS_HDFS`, `guessIsMe()`, or any `fs.defaultFS`/`HDFS_URI` beginning with `oss://`. A direct map with the explicit OSS-HDFS flag and a normal Aliyun endpoint can therefore be claimed by native OSS instead, while a native OSS map that also has `fs.defaultFS=oss://...` is accepted by both providers and depends on provider order. Please mirror the typed detection here, make native OSS explicitly decline those OSS-HDFS flags, and gate or remove the bare `oss://` fallback so the predicates stay disjoint. ########## fe/fe-filesystem/fe-filesystem-oss-hdfs/src/main/java/org/apache/doris/filesystem/hdfs/OssHdfsFileSystemProvider.java: ########## @@ -0,0 +1,79 @@ +// 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.filesystem.hdfs; + +import org.apache.doris.filesystem.FileSystem; +import org.apache.doris.filesystem.hdfs.properties.OssHdfsProperties; +import org.apache.doris.filesystem.properties.FileSystemProperties; +import org.apache.doris.filesystem.spi.FileSystemProvider; + +import java.io.IOException; +import java.util.Locale; +import java.util.Map; + +/** + * SPI provider for Aliyun OSS-HDFS (JindoFS), addressed with {@code oss://} URIs and served by an + * HDFS-compatible {@code DFSFileSystem}. Registered via META-INF/services alongside + * {@link HdfsFileSystemProvider}. + * + * <p>Routing is kept strictly disjoint from plain HDFS: the authoritative {@code _STORAGE_TYPE_} + * marker set by fe-core's StoragePropertiesConverter wins, and the heuristic fallback keys on + * {@link OssHdfsProperties#guessIsMe} (oss.hdfs flag / oss-dls endpoint) or an {@code oss://} URI. + * The framework selects providers first-match-wins over an unordered list, so the two providers' + * {@code supports()} predicates must not overlap.</p> + */ +public class OssHdfsFileSystemProvider implements FileSystemProvider<FileSystemProperties> { Review Comment: This new provider never overrides `sensitivePropertyKeys()`, so the plugin manager registers an empty set for OSS-HDFS. `DatasourcePrintableMap` relies on those provider-registered aliases to mask properties from SHOW/error output, and the SPI docs say typed providers should return `ConnectorPropertiesUtils.getSensitiveKeys(...)`. The property class marks access/secret/security token aliases sensitive, but without an override they are not registered. Please add the same override as S3/OSS providers, and include the normalized `fs.oss.*` aliases too if this plugin is made idempotent for converted backend maps. -- 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]
