hudi-agent commented on code in PR #18805:
URL: https://github.com/apache/hudi/pull/18805#discussion_r3463458928
##########
hudi-hadoop-common/src/main/java/org/apache/hudi/common/config/DFSPropertiesConfiguration.java:
##########
@@ -195,7 +219,9 @@ public void addPropsFromStream(BufferedReader reader,
StoragePath cfgFilePath) t
&& cfgFilePath != null) {
providedPath = new StoragePath(cfgFilePath.getParent(), split[1]);
}
- addPropsFromFile(providedPath);
+ // include= references may legitimately point to optional files
(e.g. environment-
+ // specific overrides); skip silently when missing rather than
failing the whole load.
+ addPropsFromFile(providedPath, true);
Review Comment:
🤖 The `include=` path now passes `tolerateMissing=true`, so a missing target
is skipped at `debug` only — the same level as the commonly-absent global
`hudi-defaults.conf`. A typo'd or mis-deployed include silently drops its
config with no visible signal, and the PR description mentions promoting this
to `warn` "so missing user-configured property files are visible." Was `debug`
intentional here, or worth logging `include=` misses at `warn` while keeping
the optional global paths at `debug`?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-hadoop-common/src/main/java/org/apache/hudi/common/config/DFSPropertiesConfiguration.java:
##########
@@ -118,15 +118,18 @@ public static TypedProperties loadGlobalProps() {
String.format("Failed to read %s from class loader",
DEFAULT_PROPERTIES_FILE), ioe);
}
}
- // Try loading the external config file from local file system
+ // Try loading the external config file from local file system. Both
DEFAULT_PATH and
+ // HUDI_CONF_DIR are optional global config locations — use the tolerant
overload so a
+ // missing file does not propagate as an exception (preserves prior
behavior covered by
+ // testClassInitializationNeverThrows).
Review Comment:
🤖 nit: could you avoid hard-coding the test method name here? If
`testClassInitializationNeverThrows` gets renamed, this comment silently rots.
Something like "preserves prior silent-ignore behavior for optional
global-defaults paths" conveys the same intent without the coupling.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-hadoop-common/src/main/java/org/apache/hudi/common/config/DFSPropertiesConfiguration.java:
##########
@@ -156,8 +156,8 @@ public void addPropsFromFile(StoragePath filePath) {
);
try {
- if (filePath.equals(DEFAULT_PATH) && !storage.exists(filePath)) {
- log.debug("Properties file {} not found. Ignoring to load props file",
filePath);
+ if (!storage.exists(filePath)) {
Review Comment:
🤖 Good catch on line 129. Worth adding that the asymmetry already exists
pre-change: loadGlobalProps wraps the DEFAULT_PATH call (lines 122-126) in a
try/catch that logs-and-continues, but the env-conf-path call on line 129 is
unwrapped, so a missing HUDI_CONF_DIR/hudi-defaults.conf already propagates as
HoodieIOException today (which is exactly what
testClassInitializationNeverThrows catches). So an isInclude-style flag that
also covers the env path — or simply wrapping line 129 the way 122-126 already
does — would make both optional global sources tolerant while still keeping
fail-fast for explicit constructor --props paths.
--
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]
