github-actions[bot] commented on code in PR #65379:
URL: https://github.com/apache/doris/pull/65379#discussion_r3600142685
##########
conf/be.conf:
##########
@@ -37,6 +37,9 @@ heartbeat_service_port = 9050
brpc_port = 8060
arrow_flight_sql_port = 8050
+# Scheme used by S3 clients when the endpoint has no explicit scheme.
+s3_client_http_scheme = https
Review Comment:
[P1] Set the fresh-install default in the Cloud template too — This adds the
HTTPS override to the FE/BE templates, but Cloud installs and reads
`cloud/conf/doris_cloud.conf` independently, and that template still omits the
key. A fresh meta-service/recycler therefore keeps
`cloud/src/common/config.h`'s `http` default for bare initial-instance,
direct-API, and legacy `ObjectStoreInfoPB` endpoints; cleanup/checker/migration
can use HTTP even while a fresh BE uses HTTPS. Please add the same override to
the shipped Cloud template while retaining the compiled `http` default for
existing upgrade configs.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/connectivity/AbstractS3CompatibleConnectivityTester.java:
##########
@@ -62,7 +62,7 @@ public void testFeConnection() throws Exception {
String endpoint = properties.getEndpoint();
try (S3Client client = S3Util.buildS3Client(
- URI.create(endpoint),
+ URI.create(S3Util.buildEndpointUrl(endpoint)),
Review Comment:
[P1] Normalize the endpoint used by the real Iceberg FileIO — This fixes
only the catalog connectivity-test client. The subsequent Iceberg catalog paths
still copy the original bare endpoint into `S3FileIOProperties.ENDPOINT` in
`AbstractIcebergProperties`, `IcebergGlueMetaStoreProperties`, and
`IcebergAwsClientCredentialsProperties`; Iceberg then passes
`URI.create(endpoint)` to the AWS SDK v2 endpoint override, where a scheme-less
value fails. Consequently `test_connection=true` can pass over the configured
scheme and the actual catalog can still fail on initialization or first file
access. Please normalize all production S3FileIO endpoint writers with the same
helper and exercise the real client in a test.
##########
fe/fe-common/src/main/java/org/apache/doris/common/Config.java:
##########
@@ -3249,6 +3249,11 @@ public static int metaServiceRpcRetryTimes() {
+ "For example: s3_load_endpoint_white_list=a,b,c"})
public static String[] s3_load_endpoint_white_list = {};
+ @ConfField(description = {
+ "The scheme added to S3 endpoints without an explicit scheme.
Valid values are http and https."},
+ options = {"http", "https"})
Review Comment:
[P2] Validate the FE scheme setting instead of relying on options metadata —
`ConfField.options()` is not enforced by `ConfigBase`: String startup values
are assigned directly. Thus a typo such as `s3_client_http_scheme=ftp` starts
FE and is concatenated into bare endpoints; stage creation then fails when the
FTP connection is cast to `HttpURLConnection`, and unchecked S3 resources can
persist the unsupported scheme. Please add real startup validation for exactly
`http`/`https`, matching the BE and Cloud validators.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/StageProperties.java:
##########
@@ -155,7 +156,8 @@ public boolean isDryRun() {
* getObjectStoreInfoPB
*/
public ObjectStoreInfoPB getObjectStoreInfoPB() {
- ObjectStoreInfoPB.Builder builder =
ObjectStoreInfoPB.newBuilder().setEndpoint(properties.get(ENDPOINT))
+ ObjectStoreInfoPB.Builder builder = ObjectStoreInfoPB.newBuilder()
+ .setEndpoint(S3Util.buildEndpointUrl(properties.get(ENDPOINT)))
Review Comment:
[P1] Preserve the pre-upgrade stage protocol when persisting the fallback —
On an upgraded FE without this new config key, the in-code fallback is `http`,
so this changes a newly created bare stage into an explicit `http://` StagePB.
Before the patch the PB stayed bare and the unchanged S3/Azure filesystem
clients resolved it to HTTPS; old persisted bare stages still do so, while
otherwise identical new stages now use HTTP for head/list and COPY. The old
preflight only connected the socket and did not validate an HTTP response, so
it did not make HTTP the effective storage protocol. Please keep the old stage
behavior on upgrades, or migrate/normalize old and new StagePB records at one
provider-aware client boundary, and test legacy-bare versus new-bare records.
--
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]
