-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-52548][CORE][TESTS] Add a test case for when shuffle manager is overridden by a SparkPlugin #51246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… overridden by a SparkPlugin
core/src/test/scala/org/apache/spark/internal/plugin/PluginContainerSuite.scala
Outdated
Show resolved
Hide resolved
…tainerSuite.scala
yaooqinn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Merged to master, thank you @zhztheplayer |
|
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) |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
dongjoon-hyun
left a comment
There was a problem hiding this 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 .
|
+1, late LGTM. |
No problem. Thanks! |
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.pluginsandspark.shuffle.managerat the same time.Does this PR introduce any user-facing change?
No.
Was this patch authored or co-authored using generative AI tooling?
No.