rich7420 commented on issue #7488: URL: https://github.com/apache/datafusion-comet/issues/7488#issuecomment-6773492529
Item 3 can be worse than a leak on Spark 4.x: the resolver can end up with no usable map at all. `shuffleBlockResolver` takes the first 2- or 3-parameter constructor that `getDeclaredConstructors` returns, and the JDK does not define that order. Spark 4.x has two 2-parameter constructors, and `c.newInstance(conf, null)` gives: - `(SparkConf, Map)` on 4.0, `(SparkConf, ConcurrentHashMap)` on 4.1+: `taskIdMapsForShuffle` is `null` - `(SparkConf, BlockManager)` on 4.0: `Collections.emptyMap()` - `(SparkConf, BlockManager)` on 4.1+: a fresh map, as item 3 describes When a shuffle block migrates in, `onComplete` in `putShuffleBlockAsStream` renames the file into place and then calls `taskIdMapsForShuffle.computeIfAbsent` ([IndexShuffleBlockResolver.scala#L304-L319](https://github.com/apache/spark/blob/v4.1.3/core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockResolver.scala#L304-L319)). That throws a `NullPointerException` on `null` and an `UnsupportedOperationException` on `emptyMap`, so the block is never reported. The sending executor then stops migrating to the receiving executor and queues the block for another one, giving up after 3 failures by default. The renamed file stays on the receiving executor, untracked. I loaded `IndexShuffleBlockResolver` in a standalone JVM (JDK 17) and printed its constructor order across 30 runs per version: | Spark | `(SparkConf, Map)` / `(SparkConf, ConcurrentHashMap)` first | `(SparkConf, BlockManager)` first | 3-parameter first | |---|---|---|---| | 4.0.4 | 13 | 16 | 1 | | 4.1.3 | 3 | 27 | 0 | | 4.2.0 | 2 | 28 | 0 | I haven't checked which constructor a real executor picks, or run decommissioning end to end. Spark 3.5 has a single constructor, so it isn't affected. Changing only `case 3` to pass `taskIdMapsForShuffle` wouldn't fix this, because `.head` can still pick a 2-parameter constructor. Calling the 3-parameter constructor with `taskIdMapsForShuffle` should fix both. Is anyone working on this? If not, I can open a PR for item 3. -- 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]
