[GLUTEN-12621][CORE] Fix NoSuchElementException in untracked memory mode without off-heap size - #12622
Conversation
…ode without off-heap size setPredefinedConfigs read the off-heap size in its non-dynamic branch with the single-arg conf.getSizeAsBytes(SPARK_OFFHEAP_SIZE_KEY). checkOffHeapSettings returns early for untracked memory mode and skips the requirement that the size be set, so the key can be absent there. SparkConf.get(String) throws NoSuchElementException on a missing key, so driver plugin init crashed instead of proceeding. VeloxListenerApi.onDriverStart has the same single-arg read and runs after setPredefinedConfigs, so the failure reaches end to end. The dynamic-sizing branch already shields it by setting SPARK_OFFHEAP_SIZE_KEY to "0"; untracked mode did not. Normalize the key to 0 in the untracked branch when it is absent, mirroring the dynamic branch, so one change covers both readers. Normal mode is unaffected since checkOffHeapSettings guarantees the key is set. Add GlutenDriverPluginSuite covering untracked mode without a size and the normal path; the untracked test also asserts the raw key is readable so the downstream VeloxListenerApi path is covered.
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
Pull request overview
Fixes a driver initialization crash in gluten-core when running with untracked memory mode enabled and spark.memory.offHeap.size unset, by normalizing the missing Spark off-heap size key to 0 so downstream code paths (including Velox driver-start) can safely read it.
Changes:
- Expose
GlutenDriverPlugin.setPredefinedConfigsto package scope for direct unit testing. - In non-dynamic-sizing mode, set
spark.memory.offHeap.sizeto"0"when absent (primarily affecting untracked memory mode). - Add
GlutenDriverPluginSuitecoverage for both the untracked/no-offheap-size case and the normal configured off-heap-size case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| gluten-core/src/main/scala/org/apache/gluten/GlutenPlugin.scala | Normalizes missing spark.memory.offHeap.size to 0 and makes setPredefinedConfigs accessible for tests. |
| gluten-core/src/test/scala/org/apache/gluten/GlutenDriverPluginSuite.scala | Adds unit tests validating untracked mode no longer throws when the off-heap size key is missing, and that normal mode reads configured values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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)) { |
There was a problem hiding this comment.
we have checked the offHeap size while initing GlutenPlugin, this seems un neccesary? or we have any cases that meet this problem?
There was a problem hiding this comment.
It's still needed for untracked mode. You're right that we validate the off-heap size in checkOffHeapSettings, but that path only runs in normal mode: untracked mode returns early at L86-89 (the COLUMNAR_MEMORY_UNTRACKED branch), before the L92-102 size check, so spark.memory.offHeap.size can stay absent.
Repro: spark.gluten.memory.untracked=true with spark.memory.offHeap.size unset and dynamic sizing off → checkOffHeapSettings returns at L89 → the else-branch (main L136) calls the single-arg conf.getSizeAsBytes(SPARK_OFFHEAP_SIZE_KEY), which throws NoSuchElementException because SparkConf.get(String) reads the raw settings map and doesn't fall back to the ConfigEntry default. The added GlutenDriverPluginSuite test "setPredefinedConfigs does not throw in untracked mode without an off-heap size" fails without this change.
If it reads clearer, I can add a short comment at the untracked early-return noting the size isn't validated there.
There was a problem hiding this 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:
-
Direct call to
setPredefinedConfigs(what this PR does). Same input state and same throwing line as the real path, runs ingluten-corewith no extra setup. It just does not enter throughinit. -
Full
SparkContextboot ingluten-core.GlutenDriverPlugin.initcallsComponent.sorted(), which needs at least oneMETA-INF/gluten-componentsfile on the classpath, andgluten-coreships none (real backends live in the backend modules), so this needs a test-only dummy backend and a registration resource.Componentalso keeps a JVM-global graph with a one-shot load latch and no reset, andComponentSuiteregisters an intentional dependency cycle without cleaning it up, so a boot suite sharing the test JVM hitsUnsupportedOperationException: Cycle detected in the component graph. Making it stable needs a testing-onlyComponentreset hook, withShuffleManagerRegistry.clear()guarded bySparkTestUtil.isTestingas the precedent. -
Integration test in a backend module like
backends-velox. That module ships a realVeloxBackendcomponent file and has no graph-polluting suite, so a realspark.pluginsboot works there without any of the option 2 machinery. The cost is that it goes throughVeloxListenerApi.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.
|
Thank you @jackylee-ch |
What changes were proposed in this pull request?
GlutenDriverPlugin.setPredefinedConfigsreads the off-heap size in its non-dynamic-sizing branch with the single-argconf.getSizeAsBytes(SPARK_OFFHEAP_SIZE_KEY).checkOffHeapSettingsreturns early for untracked memory mode (spark.gluten.memory.untracked=true) and skips the requirement that the size be set, so in that mode the key can be absent.SparkConf.get(String)throwsNoSuchElementExceptionon a missing key, so driver plugin init crashes instead of proceeding under untracked semantics.The same single-arg read exists on the Velox driver-start path:
VeloxListenerApi.onDriverStartalso callsconf.getSizeAsBytes(SPARK_OFFHEAP_SIZE_KEY), and it runs aftersetPredefinedConfigs(viainitcallingComponent.onDriverStart). The dynamic-sizing branch already shields that reader by doingconf.set(SPARK_OFFHEAP_SIZE_KEY, "0"); untracked mode did not, so it was the one unshielded path.This change normalizes the key in the untracked branch: when
spark.memory.offHeap.sizeis absent, set it to0, the same way the dynamic-sizing branch does. One change covers both readers. Normal mode is unaffected becausecheckOffHeapSettingsalready guarantees the key is set there.How was this patch tested?
Added
GlutenDriverPluginSuitewith two tests: untracked mode without an off-heap size no longer throws and yields a 0 off-heap budget, and the normal path reads a configured512m. The untracked test also assertsspark.memory.offHeap.sizeis readable afterward, which is what the downstreamVeloxListenerApipath needs. The first test fails on the current code (it throwsNoSuchElementException) and passes after the fix.Closes #12621