Add Hadoop configuration directory support for Flink cross-cluster Hudi read/write - #19519
Add Hadoop configuration directory support for Flink cross-cluster Hudi read/write#19519cbg-wx wants to merge 2 commits into
Conversation
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR adds a hadoop.conf.dir Flink option so the Hadoop configuration can be loaded from a specified directory for cross-cluster reads/writes. A couple of edge cases worth double-checking in the inline comments — notably the silent fallback when the configured directory doesn't exist, and the config key living inside the hadoop. passthrough prefix. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. A couple of small readability suggestions below, mainly around the unexplained magic string removal and minor code duplication in the test helpers.
| public static org.apache.hadoop.conf.Configuration getHadoopConf(String hadoopConfDir) { | ||
| org.apache.hadoop.conf.Configuration hadoopConf; | ||
| hadoopConf = getHadoopConfiguration(hadoopConfDir); | ||
| if (hadoopConf == null){ |
There was a problem hiding this comment.
🤖 When the directory doesn't exist (or is mistyped), getHadoopConfiguration returns null and this falls back to a bare new Configuration(). Since HadoopConfigurations.getHadoopConf then replaces the env-derived conf with this result, a typo in hadoop.conf.dir would silently use the default/local cluster config instead of the intended remote one — for a cross-cluster write that could route data to the wrong cluster with no error. Could you log a warning (or fail) when the configured dir doesn't exist rather than silently falling back?
| String hadoopConfDir = conf.getString(FlinkOptions.HADOOP_CONF_DIR.key(), null); | ||
| if (hadoopConfDir != null && !hadoopConfDir.isEmpty()) { | ||
| hadoopConf = FlinkClientUtil.getHadoopConf(hadoopConfDir); | ||
| } |
There was a problem hiding this comment.
🤖 The hadoop.conf.dir key falls inside the hadoop. passthrough prefix, so it also gets picked up by getPropertiesWithPrefix(..., HADOOP_PREFIX) as conf.dir, forcing this options.remove("conf.dir") workaround. That coupling is easy to break later. Was naming it outside the prefix (e.g. under a hoodie./flink-specific namespace) considered so no special-casing is needed?
| hadoopConf = FlinkClientUtil.getHadoopConf(hadoopConfDir); | ||
| } | ||
| Map<String, String> options = FlinkOptions.getPropertiesWithPrefix(conf.toMap(), HADOOP_PREFIX); | ||
| options.remove("conf.dir"); |
There was a problem hiding this comment.
🤖 nit: the magic string "conf.dir" here is easy to misread — could you add an inline comment explaining that this strips the key left over after the hadoop. prefix is removed from hadoop.conf.dir? Something like // remove "conf.dir" — the residual key after stripping the "hadoop." prefix from hadoop.conf.dir.
| } | ||
|
|
||
| public static org.apache.hadoop.conf.Configuration getHadoopConf(String hadoopConfDir) { | ||
| org.apache.hadoop.conf.Configuration hadoopConf; |
There was a problem hiding this comment.
🤖 nit: could you combine the declaration and assignment into a single line? org.apache.hadoop.conf.Configuration hadoopConf = getHadoopConfiguration(hadoopConfDir); reads more naturally.
| /** | ||
| * Writes a minimal core-site.xml with the given key/value into the specified directory. | ||
| */ | ||
| private void writeCoreSite(File confDir, String key, String value) throws IOException { |
There was a problem hiding this comment.
🤖 nit: writeCoreSite and writeHdfsSite are identical except for the filename — have you considered collapsing them into a single writeSiteXml(File dir, String fileName, String key, String value) helper?
Add hadoop.conf.dir in FlinkOptions to support for Flink cross-cluster Hudi read/write
Describe the issue this Pull Request addresses
This pull request in order to close #19191
Summary and Changelog
Add HADOOP_CONF_DIR to FlinkOptions
FlinkClientUtil.java— Add an overloadedgetHadoopConf(String hadoopConfDir)method withhadoop.conf.dirsupport.HadoopConfigurations.java— Update getHadoopConf to supporthadoop.conf.dirImpact
flink read/write hudi table
Risk Level
Documentation Update
Contributor's checklist