[SPARK-53337][CORE] Ensure the application name get escaped. #52084
Conversation
|
The code looks good and straightforward, could you answer the questions in PR description? |
|
Done |
|
https://github.com/apache/spark/pull/52084/checks?check_run_id=48507003442 
It seems that the Github Actions has not been executed yet. |
|
My bad, fixed |
|
https://github.com/roddhjav/spark/actions/runs/17212259314/job/48827031130 Could you retrigger the failed GA task? @roddhjav |
|
Done, one of the tests keeps failing. However, it does not look like it is related with this PR. |
There was a problem hiding this comment.
Could you provide a valid test case to protect your contribution from a future regression, please, @roddhjav ?
| @@ -345,7 +346,7 @@ class SparkConf(loadDefaults: Boolean) | |||
|
|
|||
| /** Set a name for your application. Shown in the Spark web UI. */ | |||
| def setAppName(name: String): SparkConf = { | |||
| set("spark.app.name", name) | |||
| set("spark.app.name", StringEscapeUtils.escapeHtml4(name)) | |||
There was a problem hiding this comment.
I'm not sure this is a safe change. Since spark.app.name is used in many places inside and outside of Apache Spark, this kind of implicit change may cause significant breaking changes due to the mismatches.
Let me ask in this way: if we throws Exception here, do you think it's okay? IMO, it's a breaking change, isn't it?
From my side, I'd like to recommend @roddhjav to escape in WebUI part (during HTML generation) instead of taking a risk to affect all Spark ecosystem.
There was a problem hiding this comment.
Agree with @dongjoon-hyun.
Instead, it would be better to simply escape this in Spark UI, so that what gets returned works for html (note, not for REST calls - just the html).
There was a problem hiding this comment.
In addition, I'd like to confirm whether the escaped "app name" (if it has been escaped) will affect the search functionality on the job list page in Yarn mode, and whether it is necessary to use the escaped app name for searching instead?
|
cc @cloud-fan , @mridulm , @yaooqinn too. |
Can you show us a bad case you've met? Based on a local test below, it looks like the AS-IS behavior makes sense to me without escaping. scala> org.apache.commons.text.StringEscapeUtils.escapeHtml4("ß")
val res0: String = ßbin/spark-shell --name ß
|
What changes were proposed in this pull request?
In spark web, the application name need to be escaped.
Why are the changes needed?
Not escaped app name could lead to bad thing in the web ui
Does this PR introduce any user-facing change?
No
How was this patch tested?
Tests already exist, it was manually tested.
Was this patch authored or co-authored using generative AI tooling?
No