Copilot commented on code in PR #56579:
URL: https://github.com/apache/doris/pull/56579#discussion_r2386601461
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/storage/AbstractS3CompatibleProperties.java:
##########
@@ -264,8 +254,12 @@ public void initializeHadoopStorageConfig() {
private void appendS3HdfsProperties(Configuration hadoopStorageConfig) {
hadoopStorageConfig.set("fs.s3.impl",
"org.apache.hadoop.fs.s3a.S3AFileSystem");
hadoopStorageConfig.set("fs.s3a.impl",
"org.apache.hadoop.fs.s3a.S3AFileSystem");
- hadoopStorageConfig.set("fs.s3a.endpoint", getEndpoint());
- hadoopStorageConfig.set("fs.s3a.endpoint.region", getRegion());
+ if (StringUtils.isNotBlank(getEndpoint())) {
+ hadoopStorageConfig.set("fs.s3a.endpoint", getEndpoint());
+ }
+ if (StringUtils.isNotBlank(getRegion())) {
Review Comment:
The conditional checks for endpoint and region should be consistent with the
validation logic. Since `isEndpointCheckRequired()` can return false for some
implementations (like S3Properties), but the endpoint is still set
unconditionally in the Hadoop config when not blank, this could lead to
inconsistent behavior.
```suggestion
if (isEndpointCheckRequired() &&
StringUtils.isNotBlank(getEndpoint())) {
hadoopStorageConfig.set("fs.s3a.endpoint", getEndpoint());
}
if (isEndpointCheckRequired() &&
StringUtils.isNotBlank(getRegion())) {
```
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/storage/OSSProperties.java:
##########
@@ -247,7 +249,7 @@ protected void setEndpointIfPossible() {
@Override
public void initNormalizeAndCheckProps() {
super.initNormalizeAndCheckProps();
- if (endpoint.contains("dlf") || endpoint.contains("oss-dls")) {
+ if (StringUtils.isBlank(endpoint) ||
!STANDARD_ENDPOINT_PATTERN.matcher(endpoint).matches()) {
Review Comment:
The logic change from checking for 'dlf' or 'oss-dls' strings to using
pattern matching is more robust, but the condition
`StringUtils.isBlank(endpoint)` will always result in setting the endpoint even
when one was provided. This could override user-specified custom endpoints
unexpectedly.
```suggestion
if (StringUtils.isBlank(endpoint)) {
```
--
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]
