Skip to content

Conversation

@yaooqinn
Copy link
Member

@yaooqinn yaooqinn commented Nov 5, 2025

What changes were proposed in this pull request?

Deprecated spark.shuffle.server.finalizeShuffleMergeThreadsPercent first, and then we can remove finalizeWorker usages in the future.

Why are the changes needed?

In the Netty project, those related APIs are

Does this PR introduce any user-facing change?

no

How was this patch tested?

I verified this by a SparkPi local test

bin/run-example -c spark.shuffle.server.finalizeShuffleMergeThreadsPercent=10 SparkPi
WARNING: Using incubator modules: jdk.incubator.vector
Using Spark's default log4j profile: org/apache/spark/log4j2-defaults.properties
25/11/05 15:22:14 WARN SparkConf: The configuration key 'spark.shuffle.server.finalizeShuffleMergeThreadsPercent' has been deprecated as of Spark 4.2.0 and may be removed in the future. Using separate finalizeWorkers could be problematic due to the underlying netty layer

Was this patch authored or co-authored using generative AI tooling?

no

@github-actions github-actions bot added the CORE label Nov 5, 2025
@dongjoon-hyun
Copy link
Member

Merged to master for Apache Spark 4.2.0.

@yaooqinn yaooqinn deleted the SPARK-54193 branch November 18, 2025 02:50
huangxiaopingRD pushed a commit to huangxiaopingRD/spark that referenced this pull request Nov 25, 2025
…eMergeThreadsPercent

### What changes were proposed in this pull request?

Deprecated spark.shuffle.server.finalizeShuffleMergeThreadsPercent first, and then we can remove finalizeWorker usages in the future.

### Why are the changes needed?
In the Netty project, those related APIs are
- removed from the main branch netty/netty#8778
- and deprecated in 4.2 branch netty/netty#14538

### Does this PR introduce _any_ user-facing change?
no

### How was this patch tested?

I verified this by a SparkPi local test
```scala
bin/run-example -c spark.shuffle.server.finalizeShuffleMergeThreadsPercent=10 SparkPi
WARNING: Using incubator modules: jdk.incubator.vector
Using Spark's default log4j profile: org/apache/spark/log4j2-defaults.properties
25/11/05 15:22:14 WARN SparkConf: The configuration key 'spark.shuffle.server.finalizeShuffleMergeThreadsPercent' has been deprecated as of Spark 4.2.0 and may be removed in the future. Using separate finalizeWorkers could be problematic due to the underlying netty layer
```

### Was this patch authored or co-authored using generative AI tooling?
no

Closes apache#52893 from yaooqinn/SPARK-54193.

Authored-by: Kent Yao <yao@apache.org>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
@mridulm
Copy link
Contributor

mridulm commented Dec 6, 2025

@yaooqinn , @dongjoon-hyun I did not quite understand the rationale for deprecating these configs.
The functionality which they provide is required for production stability - is the deprecation simply due to netty master not supporting these ?

If yes, what is the current alternative proposed in netty for existing usages ?

@dongjoon-hyun
Copy link
Member

To @mridulm , Netty community decided the functionality like addLast is unstable for the production due to the race condition. AFAIK, it has several issues before in Netty layer although we didn't get the error reports in Apache community yet because this is fairly new feature of Apache Spark 4.0.0, SPARK-43987

channel.pipeline().addLast(finalizeWorkers, FinalizedHandler.HANDLER_NAME,

I believe it's good to inform the users this unstable API dependency before getting this spread out further.

Maybe, we had better deprecated this at Apache Spark 4.1.0. WDTY, @mridulm ?

@dongjoon-hyun
Copy link
Member

The bottom line is that we don't want to be locked in due to this new unstable dependency of SPARK-43987.

@mridulm
Copy link
Contributor

mridulm commented Dec 8, 2025

I was not proposing locking into unstable dependency- rather explore alternatives so that we can continue to support features required for production stability of Apache Spark.

From these PR's, it appeared that deprecation of the config was only due to corresponding netty api getting deprecated/removed.

There are alternatives discussed in the netty PR - do they not work for us ? (I have not investigated - so please do point me in right direction if I am missing it !)

If they do, do we need to deprecate these configs ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants