[SPARK-44906][K8S] Make `Kubernetes[Driver|Executor]Conf.annotations` substitute annotations instead of feature steps #42600

zwangsheng · 2023-08-22T06:16:43Z

What changes were proposed in this pull request?

Move Utils. SubstituteAppNExecIds logic into KubernetesConf.annotations as the default logic,

Why are the changes needed?

Easy for users to reuse, rather than to rewrite it again at the same logic.

When user write custom feature step and using annotations, before this pr, they should call Utils. SubstituteAppNExecIds once.

Does this PR introduce any user-facing change?

Yes, but no sense for user to use annotations.

How was this patch tested?

Add unit test

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

No

…netesConf.annotations

yaooqinn · 2023-08-22T07:41:55Z

...rce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala


  override def annotations: Map[String, String] = {
    KubernetesUtils.parsePrefixedKeyValuePairs(sparkConf, KUBERNETES_EXECUTOR_ANNOTATION_PREFIX)
+      .map { case (k, v) => (k, Utils.substituteAppNExecIds(v, appId, "")) }


where is the executorId?

sry, missing the executorId param, fixed.

yaooqinn · 2023-08-22T08:05:46Z

Easy for users to reuse

Is this user-facing Or just for developers

zwangsheng · 2023-08-22T08:09:49Z

Easy for users to reuse

Is this user-facing Or just for developers

Who want to build custom feature steps and use sparkConf#annotations, can be user and developers.

yaooqinn · 2023-08-22T10:29:17Z

cc @dongjoon-hyun

dongjoon-hyun · 2023-08-22T19:56:44Z

Thank you for pinging me, @yaooqinn .

dongjoon-hyun · 2023-08-22T19:59:12Z

...rce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala

+    KubernetesUtils.parsePrefixedKeyValuePairs(sparkConf, KUBERNETES_DRIVER_ANNOTATION_PREFIX).map {
+      case(k, v) =>
+        (k, Utils.substituteAppNExecIds(v, appId, ""))
+    }


If you don't mind, shall we use the consistent short style like KubernetesExecutorConf, @zwangsheng ?

dongjoon-hyun · 2023-08-22T20:07:02Z

...anagers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesConfSuite.scala

-    assert(conf.annotations === CUSTOM_ANNOTATIONS)
+    assert(conf.annotations === CUSTOM_ANNOTATIONS.map {
+      case (k, v) =>
+        (k, Utils.substituteAppNExecIds(v, conf.appId, ""))


You can merge it into one line.

- case (k, v) => - (k, Utils.substituteAppNExecIds(v, conf.appId, "")) + case (k, v) => (k, Utils.substituteAppNExecIds(v, conf.appId, ""))

dongjoon-hyun · 2023-08-22T20:07:09Z

...anagers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesConfSuite.scala

-    assert(conf.annotations === CUSTOM_ANNOTATIONS)
+    assert(conf.annotations === CUSTOM_ANNOTATIONS.map {
+      case (k, v) =>
+        (k, Utils.substituteAppNExecIds(v, conf.appId, EXECUTOR_ID))


dongjoon-hyun

Thank you for proposing this, @zwangsheng . I left a few comments.

zwangsheng · 2023-08-23T03:17:35Z

Thanks for your review @dongjoon-hyun!

Addressed, let me know if anything missed.

dongjoon-hyun · 2023-08-23T04:40:10Z

...rce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala


  override def annotations: Map[String, String] = {
    KubernetesUtils.parsePrefixedKeyValuePairs(sparkConf, KUBERNETES_DRIVER_ANNOTATION_PREFIX)
+      .map(p => (p._1, Utils.substituteAppNExecIds(p._2, appId, "")))


Sorry, but what I suggested (#42600 (comment)) is the following style instead of p => ....

case (k, v) => ...

Sorry, i misunderstood the consistent short style.

Does the following code conform

.map{ case(k, v) => (k, Utils.substituteAppNExecIds(v, appId, executorId)) }

The following?

.map { case (k, v) => (k, Utils.substituteAppNExecIds(v, appId, executorId)) }

Yes, I mean does this format conform to the consistent short style?

.map { case (k, v) => (k, Utils.substituteAppNExecIds(v, appId, executorId)) }

dongjoon-hyun · 2023-08-23T04:40:16Z

...rce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala


  override def annotations: Map[String, String] = {
    KubernetesUtils.parsePrefixedKeyValuePairs(sparkConf, KUBERNETES_EXECUTOR_ANNOTATION_PREFIX)
+      .map(p => (p._1, Utils.substituteAppNExecIds(p._2, appId, executorId)))


dongjoon-hyun · 2023-08-23T04:40:57Z

Thank you for update, @zwangsheng . It seems that there is a minor miscommunication in the above. I added two comments.

...rce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala

…ark/deploy/k8s/KubernetesConf.scala

...rce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala

…ark/deploy/k8s/KubernetesConf.scala

dongjoon-hyun

+1, LGTM. Thank you, @zwangsheng and @yaooqinn .

dongjoon-hyun · 2023-08-23T17:45:09Z

Welcome to the Apache Spark community, @zwangsheng .
I added you to the Apache Spark contributor group and assigned SPARK-44906 to you.
Thank you again for making a PR and revising according to the review comments. :)

zwangsheng · 2023-08-24T01:48:05Z

Thanks for your help and review, @dongjoon-hyun @yaooqinn.

yaooqinn · 2023-08-24T02:18:06Z

I added you to the Apache Spark contributor group and assigned SPARK-44906 to you.

Could we add this step to the merge PR script?

Welcome to the Apache Spark community

Thanks @dongjoon-hyun. As his colleague, Welcome @zwangsheng too. However, I guess he's not a first-time contributor, LOL.

dongjoon-hyun · 2023-08-24T02:38:26Z

Oh, that's a good suggestion, but it required ASF JIRA Spark PMC permission.

Could we add this step to the merge PR script?

Here is a link. Could you try that?

https://issues.apache.org/jira/plugins/servlet/project-config/SPARK/roles

dongjoon-hyun · 2023-08-24T02:41:36Z

Oh, got it.

However, I guess he's not a first-time contributor, LOL.

But, it seems that he created new JIRA ID, @yaooqinn . That's the reason why the merge script fails.

binjieyang for SPARK-44906 (this PR)
zWangSheng for SPARK-42785

yaooqinn · 2023-08-24T02:46:54Z

Oh, that's a good suggestion, but it required ASF JIRA Spark PMC permission.

It appears that the committers of the Spark Jira Project are also admins. I once added someone to the contributor role.

zwangsheng added 2 commits August 22, 2023 14:07

[SPARK-44906][K8S] Move Utils. SubstituteAppNExecIds logic into Kuber…

ec776f3

…netesConf.annotations

Add Unit test

529187a

github-actions bot added the KUBERNETES label Aug 22, 2023

yaooqinn reviewed Aug 22, 2023

View reviewed changes

fix missing executorId

a422aa1

dongjoon-hyun reviewed Aug 22, 2023

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-44906][K8S] Move Utils. SubstituteAppNExecIds logic into KubernetesConf.annotations~~ [SPARK-44906][K8S] Make Kubernetes[Driver|Executor]Conf.annotations substitute annotations instead of feature steps Aug 22, 2023

dongjoon-hyun reviewed Aug 22, 2023

View reviewed changes

fix style

be10416

dongjoon-hyun reviewed Aug 23, 2023

View reviewed changes

fix comments

796659f

yaooqinn reviewed Aug 23, 2023

View reviewed changes

...rce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala Outdated Show resolved Hide resolved

Update resource-managers/kubernetes/core/src/main/scala/org/apache/sp…

74635dc

…ark/deploy/k8s/KubernetesConf.scala

yaooqinn reviewed Aug 23, 2023

View reviewed changes

...rce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala Outdated Show resolved Hide resolved

Update resource-managers/kubernetes/core/src/main/scala/org/apache/sp…

1bfe492

…ark/deploy/k8s/KubernetesConf.scala

yaooqinn approved these changes Aug 23, 2023

View reviewed changes

dongjoon-hyun approved these changes Aug 23, 2023

View reviewed changes

dongjoon-hyun closed this in 2a3aec1 Aug 23, 2023

jshmchenxi mentioned this pull request Apr 21, 2024

[SPARK-47730][K8S] Support APP_ID and EXECUTOR_ID placeholders in labels #46149

Closed

[SPARK-44906][K8S] Make Kubernetes[Driver|Executor]Conf.annotations substitute annotations instead of feature steps #42600

[SPARK-44906][K8S] Make Kubernetes[Driver|Executor]Conf.annotations substitute annotations instead of feature steps #42600

Uh oh!

Conversation

zwangsheng commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zwangsheng Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaooqinn commented Aug 22, 2023

Uh oh!

zwangsheng commented Aug 22, 2023

Uh oh!

yaooqinn commented Aug 22, 2023

Uh oh!

dongjoon-hyun commented Aug 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

zwangsheng commented Aug 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Aug 23, 2023

Uh oh!

Uh oh!

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Aug 23, 2023

Uh oh!

zwangsheng commented Aug 24, 2023

Uh oh!

yaooqinn commented Aug 24, 2023

Uh oh!

dongjoon-hyun commented Aug 24, 2023

Uh oh!

dongjoon-hyun commented Aug 24, 2023

Uh oh!

yaooqinn commented Aug 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[SPARK-44906][K8S] Make `Kubernetes[Driver|Executor]Conf.annotations` substitute annotations instead of feature steps #42600

[SPARK-44906][K8S] Make `Kubernetes[Driver|Executor]Conf.annotations` substitute annotations instead of feature steps #42600

zwangsheng commented Aug 22, 2023 •

edited

Loading

zwangsheng Aug 22, 2023 •

edited

Loading