Skip to content

[HUDI-4331] Allow loading external config file from class loader #5987

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yihua merged 1 commit into from
Jun 30, 2022
Merged

[HUDI-4331] Allow loading external config file from class loader #5987

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion hudi-common/src/main/java/org/apache/hudi/common/config/DFSPropertiesConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import java.io.IOException;
import java.io.InputStreamReader;
import java.net.URI;
import java.net.URL;
import java.util.HashSet;
import java.util.Set;

Expand Down Expand Up @@ -90,11 +91,24 @@ public DFSPropertiesConfiguration() {
}

/**
* Load global props from hudi-defaults.conf which is under CONF_FILE_DIR_ENV_NAME.
* Load global props from hudi-defaults.conf which is under class loader or CONF_FILE_DIR_ENV_NAME.
* @return Typed Properties
*/
public static TypedProperties loadGlobalProps() {
DFSPropertiesConfiguration conf = new DFSPropertiesConfiguration();

// First try loading the external config file from class loader
URL configFile = Thread.currentThread().getContextClassLoader().getResource(DEFAULT_PROPERTIES_FILE);
if (configFile != null) {
try (BufferedReader br = new BufferedReader(new InputStreamReader(configFile.openStream()))) {
conf.addPropsFromStream(br);
return conf.getProps();
Copy link
Contributor

@TengHuo TengHuo Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good feature that we can setup a config file from classpath.

But I think this return statement here makes it can't work with getConfPathFromEnv() together. Should it be better if we just conf.addPropsFromStream(br) here?

Then, if user setup HUDI_CONF_DIR in system env, the config file can be loaded as the old behaviour.

} catch (IOException ioe) {
throw new HoodieIOException(
Copy link
Contributor

@yihua yihua Jun 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this emit a warning message instead of erroring out?

Copy link
Contributor Author

@zhedoubushishi zhedoubushishi Jun 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a review. I feel like if a customer sets an external hudi config file but somehow Hudi fails to read it, we should terminate the job rather than continuing running it. Because this would be a job with unexpected configs.

Copy link
Contributor

@yihua yihua Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, makes sense. So the read of configs only happens when the external Hudi config file is present.

String.format("Failed to read %s from class loader", DEFAULT_PROPERTIES_FILE), ioe);
}
}
// Try loading the external config file from local file system
Option<Path> defaultConfPath = getConfPathFromEnv();
if (defaultConfPath.isPresent()) {
conf.addPropsFromFile(defaultConfPath.get());
Expand Down