Skip to content

Conversation

@zhztheplayer
Copy link
Member

@zhztheplayer zhztheplayer commented Jun 23, 2025

What changes were proposed in this pull request?

As title, adding a test case for when shuffle manager is overridden by a SparkPlugin.

Why are the changes needed?

PR #43627 introduced a change to allow the shuffle manager specified in Spark configuration to be overridden in SparkPlugin, this change was not guarded by test though.

The change in PR #43627 also allows Spark plugins to configure a shuffle manager automatically for user, so user won't have to configure both spark.plugins and spark.shuffle.manager at the same time.

Does this PR introduce any user-facing change?

No.

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

No.

@github-actions github-actions bot added the CORE label Jun 23, 2025
@zhztheplayer zhztheplayer changed the title [SPARK-52548][SQL][TESTS] Add a test case for when shuffle manager is overridden by a SparkPlugin [SPARK-52548][CORE][TESTS] Add a test case for when shuffle manager is overridden by a SparkPlugin Jun 23, 2025
Copy link
Member

@yaooqinn yaooqinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yaooqinn yaooqinn closed this in 9116d33 Jun 24, 2025
@yaooqinn
Copy link
Member

Merged to master, thank you @zhztheplayer

@zhztheplayer
Copy link
Member Author

Thanks for reviewing! @yaooqinn

override def driverPlugin(): DriverPlugin = {
new DriverPlugin {
override def init(sc: SparkContext, ctx: PluginContext): JMap[String, String] = {
sc.conf.set(SHUFFLE_MANAGER, classOf[MyShuffleManager].getName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Does this DriverPlugin also guarantee Executor-side shuffle manager, @zhztheplayer ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, the configuration modification here should always take effect on executors, similar practices are adopted a lot in our use case. I haven't set up a real cluster to test this specific option though.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding this test coverage, @zhztheplayer .

@dongjoon-hyun
Copy link
Member

+1, late LGTM.

@zhztheplayer
Copy link
Member Author

Thank you for adding this test coverage, @zhztheplayer .

No problem. Thanks!

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