morningman commented on code in PR #53571:
URL: https://github.com/apache/doris/pull/53571#discussion_r2217096989
##########
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:
+1
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergExternalCatalog.java:
##########
@@ -51,7 +55,21 @@ public IcebergExternalCatalog(long catalogId, String name,
String comment) {
}
// Create catalog based on catalog type
- protected abstract void initCatalog();
+ protected void initCatalog() {
+ try {
+ AbstractIcebergProperties properties = (AbstractIcebergProperties)
MetastoreProperties
+ .create(getProperties());
+ properties.initialize(getName(), new
ArrayList<>(catalogProperty.getStoragePropertiesMap().values()));
Review Comment:
Why the `StorageProperty` is got from `CatalogProperty`, but
`MetastoreProperties` is created here, not from `CatalogProperty`.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/MetastoreProperties.java:
##########
@@ -45,6 +45,7 @@ public class MetastoreProperties extends ConnectionProperties
{
public enum Type {
HMS("hms"),
+ ICEBERG("iceberg"),
Review Comment:
these type are not on the same level
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/IcebergPropertiesFactory.java:
##########
@@ -0,0 +1,55 @@
+// 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.commons.lang3.StringUtils;
+
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.function.Function;
+
+public class IcebergPropertiesFactory implements MetastorePropertiesFactory {
+ private static final Map<String, Function<Map<String, String>,
MetastoreProperties>> REGISTERED_SUBTYPES =
+ new HashMap<>();
+
+ static {
+ register("glue", IcebergGlueMetaStoreProperties::new);
+ //register("rest", HMSProperties::new);
Review Comment:
what about rest?
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/storage/MinioProperties.java:
##########
@@ -75,4 +75,18 @@ public static boolean guessIsMe(Map<String, String>
origProps) {
protected Set<Pattern> endpointPatterns() {
return
ImmutableSet.of(Pattern.compile("^(?:https?://)?[a-zA-Z0-9.-]+(?::\\d+)?$"));
}
+
+ @Override
+ public void initializeHadoopStorageConfig() {
+ hadoopStorageConfig = new Configuration();
+ hadoopStorageConfig.set("fs.s3.impl",
"org.apache.hadoop.fs.s3a.S3AFileSystem");
Review Comment:
Do we need hadoop prop to access minio?
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/storage/OSSHdfsProperties.java:
##########
@@ -239,4 +239,9 @@ private String validateUri(String uri) throws UserException
{
public String getStorageName() {
return "OSSHDFS";
}
+
+ @Override
+ public void initializeHadoopStorageConfig() {
+ hadoopStorageConfig = configuration;
Review Comment:
This code is very strange.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergExternalCatalog.java:
##########
@@ -51,7 +55,21 @@ public IcebergExternalCatalog(long catalogId, String name,
String comment) {
}
// Create catalog based on catalog type
- protected abstract void initCatalog();
+ protected void initCatalog() {
Review Comment:
The subclass(dlf and rest catalog) still override this method?
##########
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;
Review Comment:
this instance should be with ExternalCatalog
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AbstractIcebergProperties.java:
##########
@@ -0,0 +1,87 @@
+// 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.property.ConnectorProperty;
+import org.apache.doris.datasource.property.storage.StorageProperties;
+
+import lombok.Getter;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.catalog.Catalog;
+
+import java.util.List;
+import java.util.Map;
+
+/**
+ * @See org.apache.iceberg.CatalogProperties
+ */
+public abstract class AbstractIcebergProperties extends MetastoreProperties {
+
+ @ConnectorProperty(
+ names = {CatalogProperties.WAREHOUSE_LOCATION},
+ required = false,
+ description = "The location of the Iceberg warehouse. This is
where the tables will be stored."
+ )
+ protected String warehouse;
+
+ /**
+ * Iceberg Catalog instance responsible for managing metadata and
lifecycle of Iceberg tables.
+ * <p>
+ * The Catalog is a core component in Iceberg that handles table
registration,
+ * loading, and metadata management.
+ * <p>
+ * It is assigned during initialization via the `initialize` method,
+ * which calls the abstract `initCatalog` method to create a concrete
Catalog instance.
+ * This instance is typically configured based on the provided catalog name
+ * and a list of storage properties.
+ * <p>
+ * After initialization, the catalog must not be null; otherwise,
+ * an IllegalStateException is thrown to ensure that subsequent operations
+ * on Iceberg tables have a valid Catalog reference.
+ * <p>
+ * Different Iceberg Catalog implementations (such as HadoopCatalog,
HiveCatalog,
+ * RESTCatalog, etc.) can be flexibly switched and configured
+ * by subclasses overriding the `initCatalog` method.
+ * <p>
+ * This field is used to perform metadata operations like creating,
querying,
+ * and deleting Iceberg tables.
+ */
+ @Getter
+ protected Catalog catalog;
Review Comment:
Do not put catalog instance in this class.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AbstractIcebergProperties.java:
##########
@@ -0,0 +1,87 @@
+// 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.property.ConnectorProperty;
+import org.apache.doris.datasource.property.storage.StorageProperties;
+
+import lombok.Getter;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.catalog.Catalog;
+
+import java.util.List;
+import java.util.Map;
+
+/**
+ * @See org.apache.iceberg.CatalogProperties
+ */
+public abstract class AbstractIcebergProperties extends MetastoreProperties {
+
+ @ConnectorProperty(
+ names = {CatalogProperties.WAREHOUSE_LOCATION},
+ required = false,
+ description = "The location of the Iceberg warehouse. This is
where the tables will be stored."
+ )
+ protected String warehouse;
+
+ /**
+ * Iceberg Catalog instance responsible for managing metadata and
lifecycle of Iceberg tables.
+ * <p>
+ * The Catalog is a core component in Iceberg that handles table
registration,
+ * loading, and metadata management.
+ * <p>
+ * It is assigned during initialization via the `initialize` method,
+ * which calls the abstract `initCatalog` method to create a concrete
Catalog instance.
+ * This instance is typically configured based on the provided catalog name
+ * and a list of storage properties.
+ * <p>
+ * After initialization, the catalog must not be null; otherwise,
+ * an IllegalStateException is thrown to ensure that subsequent operations
+ * on Iceberg tables have a valid Catalog reference.
+ * <p>
+ * Different Iceberg Catalog implementations (such as HadoopCatalog,
HiveCatalog,
+ * RESTCatalog, etc.) can be flexibly switched and configured
+ * by subclasses overriding the `initCatalog` method.
+ * <p>
+ * This field is used to perform metadata operations like creating,
querying,
+ * and deleting Iceberg tables.
+ */
+ @Getter
+ protected Catalog catalog;
+
+ public abstract String getIcebergCatalogType();
+
+ protected AbstractIcebergProperties(Type type, Map<String, String> props) {
Review Comment:
This method is unused
--
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]
