vinothchandar commented on a change in pull request #3416:
URL: https://github.com/apache/hudi/pull/3416#discussion_r745743697
##########
File path:
hudi-common/src/main/java/org/apache/hudi/common/config/DFSPropertiesConfiguration.java
##########
@@ -117,7 +142,50 @@ public void addProperties(BufferedReader reader) throws
IOException {
}
}
- public TypedProperties getConfig() {
- return props;
+ public static TypedProperties getGlobalProps() {
+ final TypedProperties globalProps = new TypedProperties();
+ globalProps.putAll(HUDI_CONF_PROPS);
+ return globalProps;
+ }
+
+ public TypedProperties getProps() {
+ return getProps(false);
+ }
+
+ public TypedProperties getProps(boolean includeHudiConf) {
+ if (includeHudiConf) {
+ TypedProperties mergedProps = new TypedProperties();
+ mergedProps.putAll(HUDI_CONF_PROPS);
+ mergedProps.putAll(props);
+ return mergedProps;
+ } else {
+ return props;
+ }
+ }
+
+ private static Path getDefaultConfPath() {
+ String confDir = System.getenv(CONF_FILE_DIR_ENV_NAME);
+ if (confDir == null) {
+ LOG.warn("Cannot find " + CONF_FILE_DIR_ENV_NAME + ", please set it as
the dir of " + DEFAULT_PROPERTIES_FILE);
+ return null;
+ }
+ return new Path("file://" + confDir + File.separator +
DEFAULT_PROPERTIES_FILE);
+ }
+
+ private String[] splitProperty(String line) {
+ line = line.replaceAll("\\s+"," ");
+ String delimiter = line.contains("=") ? "=" : " ";
+ int ind = line.indexOf(delimiter);
+ String k = line.substring(0, ind).trim();
+ String v = line.substring(ind + 1).trim();
+ return new String[] {k, v};
+ }
+
+ private boolean isValidLine(String line) {
Review comment:
add a UT for the regex check?
##########
File path:
hudi-common/src/main/java/org/apache/hudi/common/config/DFSPropertiesConfiguration.java
##########
@@ -43,70 +46,92 @@
private static final Logger LOG =
LogManager.getLogger(DFSPropertiesConfiguration.class);
+ private static final String DEFAULT_PROPERTIES_FILE = "hudi-defaults.conf";
+
+ public static final String CONF_FILE_DIR_ENV_NAME = "HUDI_CONF_DIR";
+
+ // props read from hudi-defaults.conf
+ private static final TypedProperties HUDI_CONF_PROPS = loadGlobalProps();
+
private final FileSystem fs;
- private final Path rootFile;
+ private Path currentFilePath;
+ // props read from user defined configuration file or input stream
private final TypedProperties props;
// Keep track of files visited, to detect loops
- private final Set<String> visitedFiles;
+ private final Set<String> visitedFilePaths;
- public DFSPropertiesConfiguration(FileSystem fs, Path rootFile,
TypedProperties defaults) {
+ public DFSPropertiesConfiguration(FileSystem fs, Path filePath) {
this.fs = fs;
- this.rootFile = rootFile;
- this.props = defaults;
- this.visitedFiles = new HashSet<>();
- visitFile(rootFile);
- }
-
- public DFSPropertiesConfiguration(FileSystem fs, Path rootFile) {
- this(fs, rootFile, new TypedProperties());
+ this.currentFilePath = filePath;
+ this.props = new TypedProperties();
+ this.visitedFilePaths = new HashSet<>();
+ addPropsFromFile(filePath);
}
public DFSPropertiesConfiguration() {
this.fs = null;
- this.rootFile = null;
+ this.currentFilePath = null;
this.props = new TypedProperties();
- this.visitedFiles = new HashSet<>();
+ this.visitedFilePaths = new HashSet<>();
}
- private String[] splitProperty(String line) {
- int ind = line.indexOf('=');
- String k = line.substring(0, ind).trim();
- String v = line.substring(ind + 1).trim();
- return new String[] {k, v};
+ /**
+ * Load global props from hudi-defaults.conf which is under
CONF_FILE_DIR_ENV_NAME.
+ * @return Typed Properties
+ */
+ public static TypedProperties loadGlobalProps() {
+ DFSPropertiesConfiguration conf = new DFSPropertiesConfiguration();
+ Path defaultConfPath = getDefaultConfPath();
+ if (defaultConfPath != null) {
+ conf.addPropsFromFile(defaultConfPath);
+ }
+ return conf.getProps();
}
- private void visitFile(Path file) {
+ /**
+ * Add properties from external configuration files.
+ *
+ * @param filePath File path for configuration file
+ */
+ public void addPropsFromFile(Path filePath) {
+ FileSystem fileSystem;
try {
- if (visitedFiles.contains(file.getName())) {
- throw new IllegalStateException("Loop detected; file " + file + "
already referenced");
+ fileSystem = fs != null ? fs : filePath.getFileSystem(new
Configuration());
+ } catch (IOException e) {
+ throw new IllegalArgumentException("Cannot get the file system from file
path", e);
+ }
+ try (BufferedReader reader = new BufferedReader(new
InputStreamReader(fileSystem.open(filePath)))) {
+ if (visitedFilePaths.contains(filePath.toString())) {
+ throw new IllegalStateException("Loop detected; file " + filePath + "
already referenced");
}
- visitedFiles.add(file.getName());
- BufferedReader reader = new BufferedReader(new
InputStreamReader(fs.open(file)));
- addProperties(reader);
+ visitedFilePaths.add(filePath.toString());
+ currentFilePath = filePath;
+ addPropsFromBufferedReader(reader);
} catch (IOException ioe) {
- LOG.error("Error reading in properies from dfs", ioe);
+ LOG.error("Error reading in properties from dfs", ioe);
throw new IllegalArgumentException("Cannot read properties from dfs",
ioe);
}
}
/**
- * Add properties from input stream.
- *
+ * Add properties from buffered reader.
+ *
* @param reader Buffered Reader
* @throws IOException
*/
- public void addProperties(BufferedReader reader) throws IOException {
+ public void addPropsFromBufferedReader(BufferedReader reader) throws
IOException {
Review comment:
i feel name is bit too verbose now. esp the `BufferedReader` part?
##########
File path:
hudi-common/src/main/java/org/apache/hudi/common/config/DFSPropertiesConfiguration.java
##########
@@ -117,7 +142,50 @@ public void addProperties(BufferedReader reader) throws
IOException {
}
}
- public TypedProperties getConfig() {
- return props;
+ public static TypedProperties getGlobalProps() {
+ final TypedProperties globalProps = new TypedProperties();
+ globalProps.putAll(HUDI_CONF_PROPS);
+ return globalProps;
+ }
+
+ public TypedProperties getProps() {
+ return getProps(false);
+ }
+
+ public TypedProperties getProps(boolean includeHudiConf) {
+ if (includeHudiConf) {
+ TypedProperties mergedProps = new TypedProperties();
+ mergedProps.putAll(HUDI_CONF_PROPS);
+ mergedProps.putAll(props);
+ return mergedProps;
+ } else {
+ return props;
+ }
+ }
+
+ private static Path getDefaultConfPath() {
Review comment:
can we make this return an Option or not a null
##########
File path:
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/hudi/HoodieWriterUtils.scala
##########
@@ -102,4 +104,22 @@ object HoodieWriterUtils {
properties.putAll(mapAsJavaMap(parameters))
new HoodieConfig(properties)
}
+
+ val sparkDatasourceConfigsToTableConfigsMap = Map(
Review comment:
this is nice. thanks for doing this
##########
File path:
hudi-common/src/main/java/org/apache/hudi/common/config/DFSPropertiesConfiguration.java
##########
@@ -117,7 +142,50 @@ public void addProperties(BufferedReader reader) throws
IOException {
}
}
- public TypedProperties getConfig() {
- return props;
+ public static TypedProperties getGlobalProps() {
+ final TypedProperties globalProps = new TypedProperties();
+ globalProps.putAll(HUDI_CONF_PROPS);
+ return globalProps;
+ }
+
+ public TypedProperties getProps() {
+ return getProps(false);
+ }
+
+ public TypedProperties getProps(boolean includeHudiConf) {
+ if (includeHudiConf) {
+ TypedProperties mergedProps = new TypedProperties();
+ mergedProps.putAll(HUDI_CONF_PROPS);
+ mergedProps.putAll(props);
+ return mergedProps;
+ } else {
+ return props;
+ }
+ }
+
+ private static Path getDefaultConfPath() {
+ String confDir = System.getenv(CONF_FILE_DIR_ENV_NAME);
+ if (confDir == null) {
Review comment:
can we have a default for the CONF_FILE_DIR as well? /etc/hudi or
something? and fail gracefully if there is no conf there there
##########
File path:
hudi-common/src/main/java/org/apache/hudi/common/config/DFSPropertiesConfiguration.java
##########
@@ -43,70 +46,92 @@
private static final Logger LOG =
LogManager.getLogger(DFSPropertiesConfiguration.class);
+ private static final String DEFAULT_PROPERTIES_FILE = "hudi-defaults.conf";
+
+ public static final String CONF_FILE_DIR_ENV_NAME = "HUDI_CONF_DIR";
+
+ // props read from hudi-defaults.conf
+ private static final TypedProperties HUDI_CONF_PROPS = loadGlobalProps();
+
private final FileSystem fs;
- private final Path rootFile;
+ private Path currentFilePath;
+ // props read from user defined configuration file or input stream
private final TypedProperties props;
// Keep track of files visited, to detect loops
- private final Set<String> visitedFiles;
+ private final Set<String> visitedFilePaths;
- public DFSPropertiesConfiguration(FileSystem fs, Path rootFile,
TypedProperties defaults) {
+ public DFSPropertiesConfiguration(FileSystem fs, Path filePath) {
this.fs = fs;
- this.rootFile = rootFile;
- this.props = defaults;
- this.visitedFiles = new HashSet<>();
- visitFile(rootFile);
- }
-
- public DFSPropertiesConfiguration(FileSystem fs, Path rootFile) {
- this(fs, rootFile, new TypedProperties());
+ this.currentFilePath = filePath;
+ this.props = new TypedProperties();
+ this.visitedFilePaths = new HashSet<>();
+ addPropsFromFile(filePath);
}
public DFSPropertiesConfiguration() {
this.fs = null;
- this.rootFile = null;
+ this.currentFilePath = null;
this.props = new TypedProperties();
- this.visitedFiles = new HashSet<>();
+ this.visitedFilePaths = new HashSet<>();
}
- private String[] splitProperty(String line) {
- int ind = line.indexOf('=');
- String k = line.substring(0, ind).trim();
- String v = line.substring(ind + 1).trim();
- return new String[] {k, v};
+ /**
+ * Load global props from hudi-defaults.conf which is under
CONF_FILE_DIR_ENV_NAME.
+ * @return Typed Properties
+ */
+ public static TypedProperties loadGlobalProps() {
+ DFSPropertiesConfiguration conf = new DFSPropertiesConfiguration();
+ Path defaultConfPath = getDefaultConfPath();
+ if (defaultConfPath != null) {
+ conf.addPropsFromFile(defaultConfPath);
+ }
+ return conf.getProps();
}
- private void visitFile(Path file) {
+ /**
+ * Add properties from external configuration files.
+ *
+ * @param filePath File path for configuration file
+ */
+ public void addPropsFromFile(Path filePath) {
+ FileSystem fileSystem;
try {
- if (visitedFiles.contains(file.getName())) {
- throw new IllegalStateException("Loop detected; file " + file + "
already referenced");
+ fileSystem = fs != null ? fs : filePath.getFileSystem(new
Configuration());
Review comment:
this would ignore any configurations passed via the query engine's
context? e.g sparkContext.hadoopConfiguration?
##########
File path:
hudi-common/src/main/java/org/apache/hudi/common/config/DFSPropertiesConfiguration.java
##########
@@ -117,7 +142,50 @@ public void addProperties(BufferedReader reader) throws
IOException {
}
}
- public TypedProperties getConfig() {
- return props;
+ public static TypedProperties getGlobalProps() {
+ final TypedProperties globalProps = new TypedProperties();
+ globalProps.putAll(HUDI_CONF_PROPS);
+ return globalProps;
+ }
+
+ public TypedProperties getProps() {
+ return getProps(false);
+ }
+
+ public TypedProperties getProps(boolean includeHudiConf) {
+ if (includeHudiConf) {
Review comment:
lets avoid `Hudi` in variable names? its kind of redundant?
##########
File path:
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/TestHudiConf.scala
##########
@@ -0,0 +1,96 @@
+/*
+ * 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.spark.sql.hudi
+
+import org.apache.hadoop.conf.Configuration
+import org.apache.hadoop.fs.Path
+import org.apache.hudi.common.config.DFSPropertiesConfiguration
+import org.apache.hudi.common.model.HoodieTableType
+import org.apache.hudi.common.table.{HoodieTableConfig, HoodieTableMetaClient}
+
+import java.io.File
+import java.nio.file.{Files, Paths}
+
+class TestHudiConf extends TestHoodieSqlBase {
Review comment:
rename: TestSqlConf
##########
File path:
hudi-common/src/main/java/org/apache/hudi/common/config/DFSPropertiesConfiguration.java
##########
@@ -43,70 +46,92 @@
private static final Logger LOG =
LogManager.getLogger(DFSPropertiesConfiguration.class);
+ private static final String DEFAULT_PROPERTIES_FILE = "hudi-defaults.conf";
+
+ public static final String CONF_FILE_DIR_ENV_NAME = "HUDI_CONF_DIR";
+
+ // props read from hudi-defaults.conf
+ private static final TypedProperties HUDI_CONF_PROPS = loadGlobalProps();
Review comment:
rename: `GLOBAL_CONFS`
##########
File path:
hudi-common/src/main/java/org/apache/hudi/common/config/HoodieConfig.java
##########
@@ -167,7 +171,17 @@ public String getString(String key) {
}
public Properties getProps() {
- return props;
+ return getProps(false);
+ }
+
+ public Properties getProps(boolean includeHudiConf) {
Review comment:
any way to avoid this duplication with class above?
--
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]
