Copilot commented on code in PR #53571: URL: https://github.com/apache/doris/pull/53571#discussion_r2217063654
########## fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/IcebergFileSystemMetaStoreProperties.java: ########## @@ -0,0 +1,106 @@ +// 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.property.metastore; + +import org.apache.doris.common.security.authentication.HadoopAuthenticator; +import org.apache.doris.datasource.iceberg.IcebergExternalCatalog; +import org.apache.doris.datasource.property.storage.HdfsProperties; +import org.apache.doris.datasource.property.storage.StorageProperties; + +import lombok.Getter; +import org.apache.hadoop.conf.Configuration; +import org.apache.iceberg.CatalogProperties; +import org.apache.iceberg.catalog.Catalog; +import org.apache.iceberg.hadoop.HadoopCatalog; + +import java.io.IOException; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.function.Supplier; + +public class IcebergFileSystemMetaStoreProperties extends AbstractIcebergProperties { + @Getter + private HadoopAuthenticator hadoopAuthenticator; + + @Override + public String getIcebergCatalogType() { + return IcebergExternalCatalog.ICEBERG_HADOOP; + } + + public IcebergFileSystemMetaStoreProperties(Map<String, String> props) { + super(props); + } + + @Override + protected Catalog initCatalog(String catalogName, List<StorageProperties> storagePropertiesList) { + Configuration configuration = buildConfiguration(storagePropertiesList); + Map<String, String> catalogProps = buildCatalogProps(storagePropertiesList); + + HadoopCatalog catalog = new HadoopCatalog(); + catalog.setConf(configuration); + + if (hadoopAuthenticator != null) { + return executeWithAuth(() -> { + catalog.initialize(catalogName, catalogProps); + return catalog; + }); + } else { + catalog.initialize(catalogName, catalogProps); + } + + return catalog; + } + + private Configuration buildConfiguration(List<StorageProperties> storagePropertiesList) { + Configuration configuration = new Configuration(); + for (StorageProperties sp : storagePropertiesList) { + if (sp.getHadoopStorageConfig() != null) { + configuration.addResource(sp.getHadoopStorageConfig()); + } + } + return configuration; + } + + private Map<String, String> buildCatalogProps(List<StorageProperties> storagePropertiesList) { + Map<String, String> props = new HashMap<>(origProps); + + if (storagePropertiesList.size() == 1 && storagePropertiesList.get(0) instanceof HdfsProperties) { + HdfsProperties hdfsProps = (HdfsProperties) storagePropertiesList.get(0); + if (hdfsProps.isKerberos()) { + props.put(CatalogProperties.FILE_IO_IMPL, + "org.apache.doris.datasource.iceberg.fileio.DelegateFileIO"); + } + hadoopAuthenticator = hdfsProps.getHadoopAuthenticator(); + } + + props.put(CatalogProperties.WAREHOUSE_LOCATION, warehouse); + return props; + } + + private Catalog executeWithAuth(Supplier<Catalog> action) { + try { + return hadoopAuthenticator.doAs(() -> { + action.get(); // catalog.initialize() + return action.get(); // return catalog Review Comment: The action.get() call on line 99 executes catalog.initialize() but discards the result. Line 100 calls action.get() again, which will re-execute the initialization. This should execute once and return the catalog. ```suggestion return action.get(); // catalog.initialize() and return catalog ``` ########## fe/fe-core/src/main/java/org/apache/doris/datasource/property/storage/MinioProperties.java: ########## @@ -33,7 +34,6 @@ public class MinioProperties extends AbstractS3CompatibleProperties { @ConnectorProperty(names = {"minio.endpoint", "s3.endpoint", "AWS_ENDPOINT", "endpoint", "ENDPOINT"}, required = false, description = "The endpoint of Minio.") protected String endpoint = ""; Review Comment: [nitpick] Removed blank line affects code readability. Consider maintaining consistent spacing around field declarations. ```suggestion protected String endpoint = ""; ``` ########## fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AWSGlueMetaStoreBaseProperties.java: ########## @@ -99,18 +102,28 @@ private ParamRules buildRules() { .require(glueEndpoint, "glue.endpoint or aws.endpoint or aws.glue.endpoint is required"); } - public void check() { + public void checkAndInit() { buildRules().validate(); if (!ENDPOINT_PATTERN.matcher(glueEndpoint).matches()) { throw new IllegalArgumentException("AWS Glue properties (glue.endpoint) are not set correctly: " + glueEndpoint); } - } + if (StringUtils.isBlank(glueRegion)) { + // If region is not set, use the region from the endpoint + Matcher matcher = ENDPOINT_PATTERN.matcher(this.glueEndpoint.toLowerCase()); + if (matcher.matches()) { + // Check all possible groups for region (group 1, 2, or 3) + for (int i = 1; i <= matcher.groupCount(); i++) { + String group = matcher.group(i); + if (StringUtils.isNotBlank(group)) { + this.glueRegion = group; + return; + } + } + } - protected String getRegionFromGlueEndpoint() { - // https://glue.ap-northeast-1.amazonaws.com - // -> ap-northeast-1 - return glueEndpoint.split("\\.")[1]; + } Review Comment: [nitpick] The logic for extracting region from endpoint could be simplified. The current approach with multiple group checks is overly complex for this use case. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/property/storage/AbstractS3CompatibleProperties.java: ########## @@ -255,6 +255,12 @@ public String validateAndGetUri(Map<String, String> loadProps) throws UserExcept return S3PropertyUtils.validateAndGetUri(loadProps); } + @Override + public void initializeHadoopStorageConfig() { + throw new UnsupportedOperationException("Hadoop storage config initialization is not" + + " supported for S3 compatible storage."); Review Comment: [nitpick] Error message is split across lines inconsistently. Consider keeping the complete message on one line or using proper line continuation formatting. ```suggestion throw new UnsupportedOperationException("Hadoop storage config initialization is not supported for S3 compatible storage."); ``` ########## fe/fe-core/src/main/java/org/apache/doris/datasource/property/storage/OBSProperties.java: ########## @@ -97,4 +98,16 @@ protected Set<Pattern> endpointPatterns() { return ENDPOINT_PATTERN; } + @Override + public void initializeHadoopStorageConfig() { + hadoopStorageConfig = new Configuration(); + hadoopStorageConfig = new Configuration(); Review Comment: Duplicate initialization of hadoopStorageConfig. The Configuration object is created twice on consecutive lines. ```suggestion ``` ########## fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AWSGlueMetaStoreBaseProperties.java: ########## @@ -99,18 +102,28 @@ private ParamRules buildRules() { .require(glueEndpoint, "glue.endpoint or aws.endpoint or aws.glue.endpoint is required"); } - public void check() { + public void checkAndInit() { buildRules().validate(); if (!ENDPOINT_PATTERN.matcher(glueEndpoint).matches()) { throw new IllegalArgumentException("AWS Glue properties (glue.endpoint) are not set correctly: " + glueEndpoint); } - } + if (StringUtils.isBlank(glueRegion)) { + // If region is not set, use the region from the endpoint + Matcher matcher = ENDPOINT_PATTERN.matcher(this.glueEndpoint.toLowerCase()); + if (matcher.matches()) { + // Check all possible groups for region (group 1, 2, or 3) + for (int i = 1; i <= matcher.groupCount(); i++) { + String group = matcher.group(i); + if (StringUtils.isNotBlank(group)) { + this.glueRegion = group; + return; + } + } + } Review Comment: Incomplete method structure. The checkAndInit method appears to have malformed closing braces and incomplete logic flow. ```suggestion // If no region could be determined from the endpoint, throw an exception throw new IllegalArgumentException("AWS Glue properties (glue.region) could not be determined from the endpoint: " + glueEndpoint); ``` ########## fe/fe-core/src/main/java/org/apache/doris/datasource/property/storage/OSSProperties.java: ########## @@ -104,4 +105,18 @@ protected Set<Pattern> endpointPatterns() { return ENDPOINT_PATTERN; } + @Override + public void initializeHadoopStorageConfig() { + hadoopStorageConfig = new Configuration(); + hadoopStorageConfig = new Configuration(); Review Comment: Duplicate initialization of hadoopStorageConfig. The Configuration object is created twice on consecutive lines. ```suggestion ``` ########## fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/IcebergGlueMetaStoreProperties.java: ########## @@ -0,0 +1,103 @@ +// 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.property.metastore; + +import org.apache.doris.datasource.iceberg.IcebergExternalCatalog; +import org.apache.doris.datasource.property.storage.S3Properties; +import org.apache.doris.datasource.property.storage.StorageProperties; + +import org.apache.commons.lang3.StringUtils; +import org.apache.iceberg.CatalogProperties; +import org.apache.iceberg.aws.AwsProperties; +import org.apache.iceberg.aws.glue.GlueCatalog; +import org.apache.iceberg.aws.s3.S3FileIOProperties; +import org.apache.iceberg.catalog.Catalog; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +public class IcebergGlueMetaStoreProperties extends AbstractIcebergProperties { + + public AWSGlueMetaStoreBaseProperties glueProperties; + + public S3Properties s3Properties; + + // As a default placeholder. The path just use for 'create table', query stmt will not use it. + private static final String CHECKED_WAREHOUSE = "s3://doris"; + + public IcebergGlueMetaStoreProperties(Map<String, String> props) { + super(props); + } + + @Override + public String getIcebergCatalogType() { + return IcebergExternalCatalog.ICEBERG_GLUE; + } + + @Override + public void initNormalizeAndCheckProps() { + super.initNormalizeAndCheckProps(); + glueProperties = AWSGlueMetaStoreBaseProperties.of(origProps); + glueProperties.checkAndInit(); + s3Properties = S3Properties.of(origProps); + s3Properties.initNormalizeAndCheckProps(); + } + + @Override + protected Catalog initCatalog(String catalogName, List<StorageProperties> storageProperties) { + Map<String, String> props = prepareBaseCatalogProps(); + appendS3Props(props); + appendGlueProps(props); + + props.put("client.region", glueProperties.glueRegion); + + + if (StringUtils.isNotBlank(warehouse)) { + props.put(CatalogProperties.WAREHOUSE_LOCATION, warehouse); + }else{ Review Comment: Missing space before opening brace. Should be 'else {' following Java coding conventions. ```suggestion } else { ``` -- 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]
