LuciferYang opened a new pull request, #58103:
URL: https://github.com/apache/spark/pull/58103
### What changes were proposed in this pull request?
Bind the result of `Utils.randomize` in `LocalDirsFeatureStep.configurePod`,
so the local dirs resolved from configuration are actually shuffled.
Binding the result also makes two `toImmutableArraySeq` conversions
redundant, since `randomize` already returns a `Seq`, which in turn leaves the
`ArrayImplicits` import unused.
### Why are the changes needed?
`configurePod` calls `randomize` on the resolved dirs and discards the
return value:
```scala
val resolvedLocalDirs = Option(conf.sparkConf.getenv("SPARK_LOCAL_DIRS"))
.orElse(conf.getOption("spark.local.dir"))
.getOrElse(defaultLocalDir)
.split(",")
randomize(resolvedLocalDirs) // result dropped
localDirs = resolvedLocalDirs.toImmutableArraySeq // configured order
```
`Utils.randomize[T](seq: IterableOnce[T]): Seq[T]` is
`randomizeInPlace(seq.iterator.toArray).toImmutableArraySeq`. It shuffles a
fresh copy and returns it, so the caller observes nothing on the argument. The
genuinely mutating variant is `Utils.randomizeInPlace`. The sibling call
fifteen lines earlier, for the pod-template branch, binds the result and works.
Both calls arrived in the same commit, 9f9af2a7bf6 (SPARK-39755, "Improve
`LocalDirsFeatureStep` to randomize local directories"). An earlier revision of
that PR used `randomizeInPlace`; review suggested `Utils.randomize`, and the
next revision adopted it at both sites, binding the result at one and dropping
it at the other.
So on this branch `SPARK_LOCAL_DIRS`, the emptyDir volume list, and the
`spark-local-dir-N` to path pairing come out in configured order, identically
for every pod in an application.
One thing this PR deliberately does not claim: there is no measurable I/O or
capacity benefit. `DiskBlockManager.getFile` selects a directory by
`nonNegativeHash(filename) % localDirs.length`, which already spreads files
near-uniformly within each executor whatever order it receives, and in this
branch every path is an emptyDir this step creates, so they share one node
filesystem (or RAM under `spark.kubernetes.local.dirs.tmpfs`). What changes is
that the randomization SPARK-39755 added, and which its JIRA and release note
describe, now happens; the suite's `// SPARK-39755 : Changes the method to test
randomization` comment currently describes behavior that never occurs.
If reviewers would rather not have the randomization at all, deleting the
call as dead code is a reasonable alternative and I am happy to switch. The
argument for fixing rather than deleting is parity with the sibling branch and
with SPARK-39755's stated intent.
### Does this PR introduce _any_ user-facing change?
The order of `SPARK_LOCAL_DIRS` and of the generated emptyDir volume mounts
now varies between pods instead of matching the configured order. No API or
configuration change, and the set of directories and their mount paths are
unchanged.
### How was this patch tested?
A test added to `LocalDirsFeatureStepSuite` that runs `configurePod` ten
times with four configured dirs and requires that not every run produces the
same order. It also asserts, on each run, that the set of dirs is preserved,
that the volume mounts are named `spark-local-dir-1..4`, and that the mount
paths in order equal the `SPARK_LOCAL_DIRS` value, so a fix that scrambles the
two out of step would fail.
Confirmed to fail against the unfixed tree, deterministically rather than
flakily:
```
[info] - SPARK-XXXXX: randomize the local dirs resolved from configuration
*** FAILED *** (29 milliseconds)
[info] 1 was not greater than 1 local dirs were never reordered across 10
runs:
Set(/var/data/my-local-dir-1,/var/data/my-local-dir-2,/var/data/my-local-dir-3,/var/data/my-local-dir-4)
(LocalDirsFeatureStepSuite.scala:135)
```
(That run predated the JIRA id, hence the placeholder in the test name.)
After the fix, a false failure would need all ten runs to draw the same
permutation of four elements, about 1.4e-13.
`build/sbt -Pkubernetes 'kubernetes/testOnly
org.apache.spark.deploy.k8s.features.LocalDirsFeatureStepSuite'`:
```
[info] - Resolve to default local dir if neither env nor configuration are
set (52 milliseconds)
[info] - Use configured local dirs split on comma if provided. (3
milliseconds)
[info] - SPARK-58857: randomize the local dirs resolved from configuration
(7 milliseconds)
[info] - Use tmpfs to back default local dir (1 millisecond)
[info] - local dir on mounted volume (6 milliseconds)
[info] Run completed in 901 milliseconds.
[info] Total number of tests run: 5
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 5, failed 0, canceled 0, ignored 0, pending 0
```
Because the fix makes both branches genuinely random, I ran the suite six
times in total to check the pre-existing tests survive real reordering; all six
runs were 5/5. `kubernetes/scalastyle` and `kubernetes/Test/scalastyle` report
0 errors.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
