viirya commented on PR #56945: URL: https://github.com/apache/spark/pull/56945#issuecomment-4862897989
Thank you for the fix, @dongjoon-hyun. The motivation makes sense to me — the driver's container memory is never sized for `spark.memory.offHeap.size`, so reporting it in the Executors UI is misleading. Two issues, one of which I think needs to be addressed before merging: **1. This conflicts with SPARK-46947 (`DriverPlugin` memory override) and will break its test.** `PluginContainerSuite."memory override in plugin"` runs on `local-cluster[2,1,1024]` and its `MemoryOverridePlugin.driverPlugin().init()` sets `spark.memory.offHeap.enabled=true` / `spark.memory.offHeap.size` before the memory manager is created — SPARK-46947 delayed `initializeMemoryManager` until after the driver plugin loads precisely so this works. With this change, `isLocal` is false for `local-cluster`, so the clone overwrites the plugin's settings unconditionally and the driver's manager comes up with `tungstenMemoryMode == ON_HEAP` and `maxOffHeapStorageMemory == 0`, failing both driver-side assertions: https://github.com/apache/spark/blob/master/core/src/test/scala/org/apache/spark/internal/plugin/PluginContainerSuite.scala#L247-L248 (The core-module test job was still in progress when I looked, which is probably why CI hasn't flagged it yet.) Beyond the test itself, this is a behavior question we should decide explicitly: should a non-local driver ignore off-heap even when a `DriverPlugin` deliberately enables it? If yes, the test needs updating and the "no user-facing change" section should mention that plugins enabling off-heap on the driver are affected. If no, the override should be skipped when the plugin has touched these keys. **2. The cloned conf leaves `env.conf` and the memory manager disagreeing.** After this change, on a non-local driver `env.conf` still reports `spark.memory.offHeap.enabled=true` while the manager was built off-heap-disabled. I checked the current driver-side readers of `MEMORY_OFFHEAP_ENABLED` (column-vector path, `HashedRelation`, `TorrentBroadcast`) and none of them crosses the two sources today, so nothing breaks now — but any future driver-side code that reads the conf and then assumes the manager matches will be silently wrong. I realize mutating `env.conf` directly isn't an option since executors inherit `sc.conf.getAll` via `RetrieveSparkAppConfig`, so the clone is forced by this approach. An alternative that avoids the divergence entirely: thread the flag into the manager instead, e.g. `UnifiedMemoryManager(conf, numUsableCores, offHeapAllowed)`, consulted in the pool sizing and `tungstenMemoryMode`. That keeps `env.conf` authoritative and models "this process doesn't allow off-heap" explicitly rather than via a falsified conf. It would also mak e the interaction with issue 1 visible instead of silent. Larger diff, so I leave the trade-off to you. One small note: if the clone approach stays, a short comment on why *both* keys are set would help — `MEMORY_OFFHEAP_SIZE=0` is what actually zeroes the pools (`MemoryManager` sizes them from the size regardless of the enabled flag), while `MEMORY_OFFHEAP_ENABLED=false` avoids the `require(size > 0)` in `tungstenMemoryMode`. A future reader may otherwise drop one of them, since `HashedRelation` gets away with setting only the enabled flag. -- 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]
