wombatu-kun commented on code in PR #19904:
URL: https://github.com/apache/hudi/pull/19904#discussion_r4022628302
##########
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:
`testGetDefaultSpillableMapBasePath` has the same dependency on
`SPARK_LOCAL_DIRS` being unset but no equivalent guard, and it sorts ahead of
this method, so both of its assertions break on a machine whose shell exports
the variable. Worth clearing the three vars in a `@BeforeEach` rather than only
here.
##########
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:
`ConfigUtils.buildFileGroupReaderProperties` pins
`hoodie.memory.spillable.map.path` to
`HoodieMetadataConfig.getSplliableMapDir()`, which infers from
`FileSystemViewStorageConfig.SPILLABLE_DIR` and its hardcoded `/tmp/` default,
so every metadata-table read still spills to the container disk after this
change. Giving `SPILLABLE_DIR` the same `getDefaultSpillableMapBasePath()`
resolution would cover it - follow-up, not a blocker.
##########
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:
`ClusteringExecutionStrategy.getReaderProperties` pins this value into props
that `MultipleSparkJobExecutionStrategy.readRecordsForGroupAsRow` builds on the
driver and ships into the task closure, so executors spill to the driver's
`SPARK_LOCAL_DIRS` rather than their own. Is clustering in scope here -
dropping that `setProperty` would let each executor resolve locally, since an
explicitly configured value is already in the copied props.
--
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]
