LuciferYang commented on code in PR #12622:
URL: https://github.com/apache/gluten/pull/12622#discussion_r3654599439
##########
gluten-core/src/main/scala/org/apache/gluten/GlutenPlugin.scala:
##########
@@ -134,6 +135,15 @@ private object GlutenDriverPlugin extends Logging {
((onHeapSize - (300 * 1024 * 1024)) *
conf.getDouble(GlutenCoreConfig.DYNAMIC_OFFHEAP_SIZING_MEMORY_FRACTION.key,
0.6d)).toLong
} else {
+ // Untracked memory mode skips the off-heap size requirement in
checkOffHeapSettings, so
+ // the key may be absent here. Normalize it to 0 (mirroring the
dynamic-sizing branch
+ // above) so downstream readers that read spark.memory.offHeap.size
directly, e.g.
+ // VeloxListenerApi.onDriverStart, don't hit NoSuchElementException.
Normal mode always has
+ // the key set because checkOffHeapSettings enforced it, so this only
affects untracked
+ // mode without an explicit off-heap size.
+ if (!conf.contains(GlutenCoreConfig.SPARK_OFFHEAP_SIZE_KEY)) {
Review Comment:
To confirm this reproduces on the real driver-init path and not only in the
unit test:
With `spark.plugins=org.apache.gluten.GlutenPlugin`,
`spark.gluten.memory.untracked=true`, and `spark.memory.offHeap.size` left
unset, constructing a `SparkContext` runs `GlutenDriverPlugin.init` ->
`setPredefinedConfigs` and fails with:
```
java.util.NoSuchElementException: spark.memory.offHeap.size
```
I checked this locally by wiring a test through `new SparkContext(conf)`
with the real `spark.plugins` registration. It fails on the unfixed tree with
that exception and passes with this change.
There are three places a boot-based test could live, each with a cost:
1. Direct call to `setPredefinedConfigs` (what this PR does). Same input
state and same throwing line as the real path, runs in `gluten-core` with no
extra setup. It just does not enter through `init`.
2. Full `SparkContext` boot in `gluten-core`. `GlutenDriverPlugin.init`
calls `Component.sorted()`, which needs at least one
`META-INF/gluten-components` file on the classpath, and `gluten-core` ships
none (real backends live in the backend modules), so this needs a test-only
dummy backend and a registration resource. `Component` also keeps a JVM-global
graph with a one-shot load latch and no reset, and `ComponentSuite` registers
an intentional dependency cycle without cleaning it up, so a boot suite sharing
the test JVM hits `UnsupportedOperationException: Cycle detected in the
component graph`. Making it stable needs a testing-only `Component` reset hook,
with `ShuffleManagerRegistry.clear()` guarded by `SparkTestUtil.isTesting` as
the precedent.
3. Integration test in a backend module like `backends-velox`. That module
ships a real `VeloxBackend` component file and has no graph-polluting suite, so
a real `spark.plugins` boot works there without any of the option 2 machinery.
The cost is that it goes through `VeloxListenerApi.onDriverStart`, so it needs
the native build and only runs in CI, and the test would sit in a different
module than the one-line fix.
I lean toward option 1 for this fix, and option 3 if we want a
production-level boot test. Fine either way, tell me which you prefer.
--
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]
