morningman commented on code in PR #48086:
URL: https://github.com/apache/doris/pull/48086#discussion_r1962737962
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/CreateCatalogStmt.java:
##########
@@ -82,6 +82,7 @@ public boolean isSetIfNotExists() {
@Override
public void analyze(Analyzer analyzer) throws UserException {
+ Thread.currentThread().interrupt();
Review Comment:
Why adding this?
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/connection/PaimonConnectionProperties.java:
##########
@@ -65,7 +65,8 @@ private void initMetastoreProperties(MetastoreProperties
metaProps)
case HMS:
options.set("metastore", "hive");
HMSProperties hmsProperties = (HMSProperties) metaProps;
- hmsProperties.toPaimonOptionsAndConf(options, hadoopConf);
+ //todo we need add all metestore parameters to paimon options?
Review Comment:
```suggestion
// TODO we need add all metastore parameters to paimon
options?
```
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/HMSProperties.java:
##########
@@ -72,29 +86,50 @@ protected void checkRequiredProperties() {
+ "but service principal, client principal or client
keytab is not set.");
}
}
+ if (Strings.isNullOrEmpty(hiveMetastoreUri)) {
+ throw new IllegalArgumentException("Hive metastore uri is
required.");
+ }
}
- public void toPaimonOptionsAndConf(Options options, Configuration conf) {
- options.set("uri", hiveMetastoreUri);
+ private void checkHiveConfResourcesConfig() {
Map<String, String> allProps =
loadConfigFromFile(getResourceConfigPropName());
- allProps.forEach(conf::set);
- conf.set("hive.metastore.authentication.type",
hiveMetastoreAuthenticationType);
- if ("kerberos".equalsIgnoreCase(hiveMetastoreAuthenticationType)) {
- conf.set("hive.metastore.service.principal",
hiveMetastoreServicePrincipal);
- conf.set("hive.metastore.client.principal",
hiveMetastoreClientPrincipal);
- conf.set("hive.metastore.client.keytab",
hiveMetastoreClientKeytab);
+ if (allProps.isEmpty()) {
+ throw new IllegalArgumentException("Hive conf resources config is
not empty"
Review Comment:
I think we don't need to check this. let user guarantee this. maybe there is
only an empty xml there.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/HMSProperties.java:
##########
@@ -72,29 +86,50 @@ protected void checkRequiredProperties() {
+ "but service principal, client principal or client
keytab is not set.");
}
}
+ if (Strings.isNullOrEmpty(hiveMetastoreUri)) {
+ throw new IllegalArgumentException("Hive metastore uri is
required.");
+ }
}
- public void toPaimonOptionsAndConf(Options options, Configuration conf) {
- options.set("uri", hiveMetastoreUri);
+ private void checkHiveConfResourcesConfig() {
Map<String, String> allProps =
loadConfigFromFile(getResourceConfigPropName());
- allProps.forEach(conf::set);
- conf.set("hive.metastore.authentication.type",
hiveMetastoreAuthenticationType);
- if ("kerberos".equalsIgnoreCase(hiveMetastoreAuthenticationType)) {
- conf.set("hive.metastore.service.principal",
hiveMetastoreServicePrincipal);
- conf.set("hive.metastore.client.principal",
hiveMetastoreClientPrincipal);
- conf.set("hive.metastore.client.keytab",
hiveMetastoreClientKeytab);
+ if (allProps.isEmpty()) {
+ throw new IllegalArgumentException("Hive conf resources config is
not empty"
+ + ", but load config from file is empty.");
}
+ if (Strings.isNullOrEmpty(hiveMetastoreUri) &&
Strings.isNullOrEmpty(allProps.get(HIVE_METASTORE_URLS_KEY))) {
Review Comment:
I suggest to require user to set `hive.metastore.urls` explicitly, not
support reading from config file, that is to flexible.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/HMSProperties.java:
##########
@@ -37,6 +42,10 @@ public class HMSProperties extends MetastoreProperties {
description = "The authentication type of the hive metastore.")
private String hiveMetastoreAuthenticationType = "none";
+ @ConnectorProperty(names = {"hive.conf.resources"},
+ required = false,
+ description = "The conf resources of the hive metastore.")
+ private String hiveConfResourcesConfig = "";
Review Comment:
add empty line
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/HMSProperties.java:
##########
@@ -17,18 +17,23 @@
package org.apache.doris.datasource.property.metastore;
+import org.apache.doris.common.CatalogConfigFileUtils;
import org.apache.doris.datasource.property.ConnectorProperty;
import com.google.common.base.Strings;
+import com.google.common.collect.Maps;
import lombok.extern.slf4j.Slf4j;
-import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.conf.HiveConf;
import org.apache.paimon.options.Options;
import java.util.Map;
@Slf4j
public class HMSProperties extends MetastoreProperties {
- @ConnectorProperty(names = {"hive.metastore.uri"},
+
+ private static final String HIVE_METASTORE_URLS_KEY =
"hive.metastore.uris";
Review Comment:
add empty line
##########
fe/fe-common/src/main/java/org/apache/doris/common/Config.java:
##########
@@ -3036,6 +3036,12 @@ public class Config extends ConfigBase {
"The default directory for storing hadoop conf configuration
files."})
public static String hadoop_config_dir = EnvUtils.getDorisHome() +
"/plugins/hadoop_conf/";
+ @ConfField(description = {
+ "存放 hive conf 配置文件的默认目录。",
+ "The default directory for storing hive conf configuration
files."})
+ public static String hive_config_dir = EnvUtils.getDorisHome() +
"/plugins/hive_conf/";
Review Comment:
Can we use `hadoop_config_dir`?
--
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: commits-unsubscr...@doris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org
