vamsikarnika commented on code in PR #19904:
URL: https://github.com/apache/hudi/pull/19904#discussion_r4094457344
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/common/util/TestFileIOUtils.java:
##########
@@ -80,24 +80,73 @@ public void testReadAsUTFStringLines() {
@Test
public void testGetConfiguredLocalDirs() {
- Map<String, String> env = System.getenv();
- Class<?> clazz = env.getClass();
- Map<String, String> envMaps = null;
- try {
- Field field = clazz.getDeclaredField("m");
- field.setAccessible(true);
- envMaps = (Map<String, String>) field.get(env);
- envMaps.put("CONTAINER_ID", "xxxxx");
- } catch (NoSuchFieldException | IllegalAccessException e) {
- throw new IllegalArgumentException(e);
- }
+ Map<String, String> envMaps = mutableEnv();
+ envMaps.put("CONTAINER_ID", "xxxxx");
+ // getConfiguredLocalDirs now also consults SPARK_LOCAL_DIRS; drop any
value inherited
+ // from the developer's shell so this assertion stays about the tmpdir
fallback.
+ envMaps.remove("SPARK_LOCAL_DIRS");
Review Comment:
Good catch — `testGetDefaultSpillableMapBasePath` had no guard against an
inherited `SPARK_LOCAL_DIRS`. Fixed in 4c26f2d by moving the save/clear/restore
of `CONTAINER_ID`/`LOCAL_DIRS`/`SPARK_LOCAL_DIRS` into class-wide
`@BeforeEach`/`@AfterEach` on `TestFileIOUtils`, so every test (including this
one) gets it instead of relying on ad-hoc per-test guards.
##########
hudi-io/src/main/java/org/apache/hudi/io/util/FileIOUtils.java:
##########
@@ -269,6 +269,11 @@ public static String[] getConfiguredLocalDirs() {
// created the directories already, and that they are secured so that
only the
// user has access to them.
return getYarnLocalDirs().split(",");
+ } else if (System.getenv("SPARK_LOCAL_DIRS") != null) {
+ // Kubernetes provides no scheduler-side local-dir contract like YARN's,
so Spark itself
+ // publishes the mounted scratch paths here. Without this we fall
through to
+ // java.io.tmpdir, i.e. /tmp inside the container rather than the
mounted disk.
+ return System.getenv("SPARK_LOCAL_DIRS").split(",");
Review Comment:
Confirmed and fixed in 4c26f2d — dropped the
`props.setProperty(SPILLABLE_MAP_BASE_PATH.key(), ...)` pin in
`getReaderProperties()`. Traced the two consumers (`FileGroupRecordBuffer`,
`LsmFileGroupRecordIterator`) and both already resolve the default lazily via
`FileIOUtils.getDefaultSpillableMapBasePath()` inside the executor-side closure
when the key is absent from props, so this is a pure removal — no fallback
logic needed. Verified the plain write path (`BaseSparkCommitActionExecutor`)
and compaction (`HoodieCompactor`) don't have this bug since they resolve
`config.getSpillableMapBasePath()` inside the per-partition lambda already,
which is why this only showed up in clustering.
##########
hudi-io/src/main/java/org/apache/hudi/io/util/FileIOUtils.java:
##########
@@ -269,6 +269,11 @@ public static String[] getConfiguredLocalDirs() {
// created the directories already, and that they are secured so that
only the
// user has access to them.
return getYarnLocalDirs().split(",");
+ } else if (System.getenv("SPARK_LOCAL_DIRS") != null) {
Review Comment:
Agreed this is real, but it's a separate bug in a different config's default
(`FileSystemViewStorageConfig.SPILLABLE_DIR` hardcoded to `/tmp/`, inferred
into `HoodieMetadataConfig.SPILLABLE_MAP_DIR_PROP`) rather than anything this
PR's `FileIOUtils` change touches. Tracking it as a follow-up rather than
folding it into this PR, per your note that it's not a blocker here.
--
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]
