[SPARK-40763][K8S] Should expose driver service name to config for user features #38202
Conversation
|
@zwangsheng would you mind filing a ticket at first? |
|
Can one of the admins verify this patch? |
OK, will file jra ticket later. |
zwangsheng
changed the title
Thanks for your reminder, the corresponding unit test has been added. |
| @@ -50,6 +50,8 @@ private[spark] class DriverServiceFeatureStep( | |||
| s"$shorterServiceName as the driver service's name.") | |||
| shorterServiceName | |||
| } | |||
| // expose driver service name | |||
| kubernetesConf.sparkConf.set(RESOLVED_SPARK_DRIVER_SVC_NAME, resolvedServiceName) | |||
There was a problem hiding this comment.
Thank you for update, @zwangsheng .
BTW, this proposal looks exceptional because we have been using kubernetesConf.sparkConf as read-only until now. Is there any other instance where Apache Spark use sparkConf like this?
There was a problem hiding this comment.
I agree with you that Apache Spark haven't tried to break this read-only rule elsewhere for kubernetesConf.sparkConf, so maybe we can try adding an additional variable to kubernetesConf for contextual passing.
I will make changes to the logic inside the PR to comply with the kubernetesConf.sparkConf read-only principle.
|
In order to maintain the And move generate driver serive name code block to |
|
Gentle ping @dongjoon-hyun Hi, modified the way to expose |
|
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
What changes were proposed in this pull request?
Current on kubernetes, users use custom feature step can't perceive spark driver service name, which may generated by clock and uuid.
In some case, such as ingress, user need to know driver service name.
In this pr, we use configuration to pass it so that users are aware of it.
Of course, we want hear a better way of perceiving service name.
Why are the changes needed?
Does this PR introduce any user-facing change?
yes
How was this patch tested?
Local test