You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/03/17 06:38:24 UTC
[GitHub] [spark] dcoliversun opened a new pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVarsWithKV and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
dcoliversun opened a new pull request #35886:
URL: https://github.com/apache/spark/pull/35886
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This PR introudces `buildEnvVarsWithKV` and `buildEnvVarsWithFieldRef` functiosn for `KubernetesUtils` to eliminate duplicate code pattern.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
Code simplification. Remove redundant EnvVar Build code for driver and executor pod.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
* Add unit test and Pass the GA.
* Runing k8s integration test manaully:
```plain
$ build/sbt -Pkubernetes -Pkubernetes-integration-tests -Dtest.exclude.tags=minikube,r "kubernetes-integration-tests/test"
[info] KubernetesSuite:
[info] - Run SparkPi with no resources (18 seconds, 285 milliseconds)
[info] - Run SparkPi with no resources & statefulset allocation (18 seconds, 56 milliseconds)
[info] - Run SparkPi with a very long application name. (15 seconds, 828 milliseconds)
[info] - Use SparkLauncher.NO_RESOURCE (13 seconds, 807 milliseconds)
[info] - Run SparkPi with a master URL without a scheme. (14 seconds, 504 milliseconds)
[info] - Run SparkPi with an argument. (13 seconds, 573 milliseconds)
[info] - Run SparkPi with custom labels, annotations, and environment variables. (17 seconds, 107 milliseconds)
[info] - All pods have the same service account by default (12 seconds, 551 milliseconds)
[info] - Run extraJVMOptions check on driver (7 seconds, 452 milliseconds)
[info] - Run SparkRemoteFileTest using a remote data file (13 seconds, 910 milliseconds)
[info] - Verify logging configuration is picked from the provided SPARK_CONF_DIR/log4j2.properties (26 seconds, 493 milliseconds)
[info] - Run SparkPi with env and mount secrets. (24 seconds, 546 milliseconds)
[info] - Run PySpark on simple pi.py example (14 seconds, 609 milliseconds)
[info] - Run PySpark to test a pyfiles example (17 seconds, 710 milliseconds)
[info] - Run PySpark with memory customization (14 seconds, 918 milliseconds)
[info] - Run in client mode. (10 seconds, 385 milliseconds)
[info] - Start pod creation from template (14 seconds, 522 milliseconds)
[info] - SPARK-38398: Schedule pod creation from template (12 seconds, 771 milliseconds)
[info] - Test basic decommissioning (48 seconds, 155 milliseconds)
[info] - Test basic decommissioning with shuffle cleanup (47 seconds, 723 milliseconds)
[info] - Test decommissioning with dynamic allocation & shuffle cleanups (2 minutes, 48 seconds)
[info] - Test decommissioning timeouts (47 seconds, 866 milliseconds)
[info] - SPARK-37576: Rolling decommissioning (1 minute, 45 seconds)
[info] Run completed in 13 minutes, 51 seconds.
[info] Total number of tests run: 23
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 23, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dcoliversun commented on a change in pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVarsWithKV and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
dcoliversun commented on a change in pull request #35886:
URL: https://github.com/apache/spark/pull/35886#discussion_r828907240
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesUtilsSuite.scala
##########
@@ -127,4 +127,34 @@ class KubernetesUtilsSuite extends SparkFunSuite with PrivateMethodTester {
}
}
}
+
Review comment:
Already have. Please check
https://github.com/apache/spark/blob/46ccc22ee40c780f6ae4a9af4562fb1ad10ccd9f/resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStepSuite.scala#L51
https://github.com/apache/spark/blob/46ccc22ee40c780f6ae4a9af4562fb1ad10ccd9f/resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStepSuite.scala#L506-L535
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVarsWithKV and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #35886:
URL: https://github.com/apache/spark/pull/35886#issuecomment-1070654110
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVars and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35886:
URL: https://github.com/apache/spark/pull/35886#discussion_r841159791
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -381,4 +381,42 @@ object KubernetesUtils extends Logging {
}
}
}
+
+ /**
+ * This function builds the EnvVar objects for each key-value env.
+ */
+ @Since("3.4.0")
+ def buildEnvVars(env: Map[String, String]): Seq[EnvVar] = {
+ if (env.isEmpty) {
+ Seq.empty
+ } else {
+ env.filter( env => env._2 != null)
+ .map { env =>
+ new EnvVarBuilder()
+ .withName(env._1)
+ .withValue(env._2)
+ .build()
+ }
+ }.toSeq
+ }
+
+ /**
+ * This function builds the EnvVar objects for each field ref env.
+ */
+ @Since("3.4.0")
+ def buildEnvVarsWithFieldRef(env: Seq[(String, String, String)]): Seq[EnvVar] = {
+ if (env.isEmpty) {
Review comment:
ditto. This looks like redundant and not `Scalatic`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVars and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35886:
URL: https://github.com/apache/spark/pull/35886#issuecomment-1086775626
Sorry for being late. To prepare Apache Spark 3.3.0, I was too busy in these days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dcoliversun commented on a change in pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVarsWithKV and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
dcoliversun commented on a change in pull request #35886:
URL: https://github.com/apache/spark/pull/35886#discussion_r829818581
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -381,4 +381,42 @@ object KubernetesUtils extends Logging {
}
}
}
+
+ /**
+ * This function builds the EnvVar objects for each key-value env.
+ */
+ @Since("3.4.0")
+ def buildEnvVarsWithKV(env: Map[String, String]): Seq[EnvVar] = {
Review comment:
Thanks for your comment :) Useful for me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dcoliversun commented on a change in pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVarsWithKV and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
dcoliversun commented on a change in pull request #35886:
URL: https://github.com/apache/spark/pull/35886#discussion_r828935216
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -381,4 +381,42 @@ object KubernetesUtils extends Logging {
}
}
}
+
+ /**
+ * This function builds the EnvVar objects for each key-value env.
+ */
+ @Since("3.3.1")
+ def buildEnvVarsWithKV(env: Seq[(String, String)]): Seq[EnvVar] = {
Review comment:
Good idea. `Map` is better than `Seq` because it is kv structure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVars and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35886:
URL: https://github.com/apache/spark/pull/35886#discussion_r841159754
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -381,4 +381,42 @@ object KubernetesUtils extends Logging {
}
}
}
+
+ /**
+ * This function builds the EnvVar objects for each key-value env.
+ */
+ @Since("3.4.0")
+ def buildEnvVars(env: Map[String, String]): Seq[EnvVar] = {
+ if (env.isEmpty) {
Review comment:
I'm wondering why we need this. This doesn't look like `Scalatic` if `else` is enough to handle this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dcoliversun commented on a change in pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVarsWithKV and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
dcoliversun commented on a change in pull request #35886:
URL: https://github.com/apache/spark/pull/35886#discussion_r829821227
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesUtilsSuite.scala
##########
@@ -127,4 +127,35 @@ class KubernetesUtilsSuite extends SparkFunSuite with PrivateMethodTester {
}
}
}
+
+ test("SPARK-38582: verify that envVars built with kv env as expected") {
+ val input = for (i <- 9 to 1 by -1) yield (s"testEnvKey.$i", "testEnvValue123456")
+ val expectedEnvVars = input.map { case(k, v) =>
+ new EnvVarBuilder()
+ .withName(k)
+ .withValue(v).build()
+ }
+ val outputEnvVars =
+ KubernetesUtils.buildEnvVarsWithKV(input.toMap + ("testKeyForNull" -> null))
+ assert(outputEnvVars.toSet == expectedEnvVars.toSet)
+ }
+
+ test("SPARK-38582: verify that envVars built with field ref env as expected") {
+ val input = for (i <- 9 to 1 by -1) yield (s"testEnvKey.$i", s"v$i", "testEnvValue123456")
Review comment:
Nice. I fixed it~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dcoliversun commented on pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVarsWithKV and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
dcoliversun commented on pull request #35886:
URL: https://github.com/apache/spark/pull/35886#issuecomment-1070474609
@dongjoon-hyun
It would be good if you could take a look when you have time, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dcoliversun commented on a change in pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVarsWithKV and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
dcoliversun commented on a change in pull request #35886:
URL: https://github.com/apache/spark/pull/35886#discussion_r828932806
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -381,4 +381,42 @@ object KubernetesUtils extends Logging {
}
}
}
+
+ /**
+ * This function builds the EnvVar objects for each key-value env.
+ */
+ @Since("3.3.1")
Review comment:
Nice. I will fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dcoliversun commented on pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVars and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
dcoliversun commented on pull request #35886:
URL: https://github.com/apache/spark/pull/35886#issuecomment-1073530030
Gentle ping @dongjoon-hyun @martin-g
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dcoliversun commented on a change in pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVarsWithKV and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
dcoliversun commented on a change in pull request #35886:
URL: https://github.com/apache/spark/pull/35886#discussion_r829821107
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesUtilsSuite.scala
##########
@@ -127,4 +127,35 @@ class KubernetesUtilsSuite extends SparkFunSuite with PrivateMethodTester {
}
}
}
+
+ test("SPARK-38582: verify that envVars built with kv env as expected") {
+ val input = for (i <- 9 to 1 by -1) yield (s"testEnvKey.$i", "testEnvValue123456")
Review comment:
Nice. I fixed it~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dcoliversun commented on a change in pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVarsWithKV and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
dcoliversun commented on a change in pull request #35886:
URL: https://github.com/apache/spark/pull/35886#discussion_r828933100
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -381,4 +381,42 @@ object KubernetesUtils extends Logging {
}
}
}
+
+ /**
+ * This function builds the EnvVar objects for each key-value env.
+ */
+ @Since("3.3.1")
+ def buildEnvVarsWithKV(env: Seq[(String, String)]): Seq[EnvVar] = {
+ if (env.isEmpty) {
+ Seq.empty
+ } else {
+ env.filter( env => env._2 != null)
+ .map { env =>
+ new EnvVarBuilder()
+ .withName(env._1)
+ .withValue(env._2)
+ .build()
+ }
+ }
+ }
+
+ /**
+ * This function builds he EnvVar objects for each field ref env.
Review comment:
Nice. I will fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dcoliversun commented on pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVarsWithKV and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
dcoliversun commented on pull request #35886:
URL: https://github.com/apache/spark/pull/35886#issuecomment-1072066622
@Yikun @martin-g
Update code for comments. It's good if you have a look again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVars and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35886:
URL: https://github.com/apache/spark/pull/35886#issuecomment-1086916633
FYI, we had better hold on these kind of PRs during the planned release process. It's the same for the other refactoring PRs.
- https://github.com/apache/spark/pull/36049#pullrequestreview-929704033
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVars and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35886:
URL: https://github.com/apache/spark/pull/35886#discussion_r841159948
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStep.scala
##########
@@ -74,16 +74,8 @@ private[spark] class BasicDriverFeatureStep(conf: KubernetesDriverConf)
private val driverMemoryWithOverheadMiB = driverMemoryMiB + memoryOverheadMiB
override def configurePod(pod: SparkPod): SparkPod = {
- val driverCustomEnvs = (Seq(
- (ENV_APPLICATION_ID, conf.appId)
- ) ++ conf.environment)
- .map { env =>
- new EnvVarBuilder()
- .withName(env._1)
- .withValue(env._2)
- .build()
- }
-
+ val driverCustomEnvs = KubernetesUtils.buildEnvVars(
+ conf.environment + (ENV_APPLICATION_ID -> conf.appId))
Review comment:
Ur, are you sure about this change? This looks inconsistent to me because this breaks the existing behavior. Previously, `conf.environment`'s `ENV_APPLICATION_ID` supersedes if there is a duplication.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Yikun commented on a change in pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVarsWithKV and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35886:
URL: https://github.com/apache/spark/pull/35886#discussion_r828855941
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -381,4 +381,42 @@ object KubernetesUtils extends Logging {
}
}
}
+
+ /**
+ * This function builds the EnvVar objects for each key-value env.
+ */
+ @Since("3.3.1")
Review comment:
I think it should be `3.4.0`.
If you want to submit a PR for 3.3.1, you need first merge it in master **after 3.3.0 released published**, then backport to 3.3 branch. I guess it's not your intention.
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesUtilsSuite.scala
##########
@@ -127,4 +127,34 @@ class KubernetesUtilsSuite extends SparkFunSuite with PrivateMethodTester {
}
}
}
+
Review comment:
Looks like you have done test on utils.
It would be good if you can make sure we have exsiting UT to protected the changes on featurestep.
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -381,4 +381,42 @@ object KubernetesUtils extends Logging {
}
}
}
+
+ /**
+ * This function builds the EnvVar objects for each key-value env.
+ */
+ @Since("3.3.1")
+ def buildEnvVarsWithKV(env: Seq[(String, String)]): Seq[EnvVar] = {
Review comment:
Random thoughts: now that we're refactoring, might we think `Map` be a better choice than `Seq`?
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -381,4 +381,42 @@ object KubernetesUtils extends Logging {
}
}
}
+
+ /**
+ * This function builds the EnvVar objects for each key-value env.
+ */
+ @Since("3.3.1")
+ def buildEnvVarsWithKV(env: Seq[(String, String)]): Seq[EnvVar] = {
+ if (env.isEmpty) {
+ Seq.empty
+ } else {
+ env.filter( env => env._2 != null)
+ .map { env =>
+ new EnvVarBuilder()
+ .withName(env._1)
+ .withValue(env._2)
+ .build()
+ }
+ }
+ }
+
+ /**
+ * This function builds he EnvVar objects for each field ref env.
+ */
+ @Since("3.3.1")
Review comment:
likewise
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -381,4 +381,42 @@ object KubernetesUtils extends Logging {
}
}
}
+
+ /**
+ * This function builds the EnvVar objects for each key-value env.
+ */
+ @Since("3.3.1")
+ def buildEnvVarsWithKV(env: Seq[(String, String)]): Seq[EnvVar] = {
+ if (env.isEmpty) {
+ Seq.empty
+ } else {
+ env.filter( env => env._2 != null)
+ .map { env =>
+ new EnvVarBuilder()
+ .withName(env._1)
+ .withValue(env._2)
+ .build()
+ }
+ }
+ }
+
+ /**
+ * This function builds he EnvVar objects for each field ref env.
Review comment:
nits: s/he/the
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] martin-g commented on a change in pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVarsWithKV and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #35886:
URL: https://github.com/apache/spark/pull/35886#discussion_r829749892
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -381,4 +381,42 @@ object KubernetesUtils extends Logging {
}
}
}
+
+ /**
+ * This function builds the EnvVar objects for each key-value env.
+ */
+ @Since("3.4.0")
+ def buildEnvVarsWithKV(env: Map[String, String]): Seq[EnvVar] = {
Review comment:
```suggestion
def buildEnvVars(env: Map[String, String]): Seq[EnvVar] = {
```
IMO there is no need of `WithKV` - the passed argument is not a Seq of tuples anymore.
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesUtilsSuite.scala
##########
@@ -127,4 +127,35 @@ class KubernetesUtilsSuite extends SparkFunSuite with PrivateMethodTester {
}
}
}
+
+ test("SPARK-38582: verify that envVars built with kv env as expected") {
+ val input = for (i <- 9 to 1 by -1) yield (s"testEnvKey.$i", "testEnvValue123456")
Review comment:
```suggestion
val input = for (i <- 9 to 1 by -1) yield (s"testEnvKey.$i", s"testEnvValue.$i")
```
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesUtilsSuite.scala
##########
@@ -127,4 +127,35 @@ class KubernetesUtilsSuite extends SparkFunSuite with PrivateMethodTester {
}
}
}
+
+ test("SPARK-38582: verify that envVars built with kv env as expected") {
+ val input = for (i <- 9 to 1 by -1) yield (s"testEnvKey.$i", "testEnvValue123456")
+ val expectedEnvVars = input.map { case(k, v) =>
+ new EnvVarBuilder()
+ .withName(k)
+ .withValue(v).build()
+ }
+ val outputEnvVars =
+ KubernetesUtils.buildEnvVarsWithKV(input.toMap + ("testKeyForNull" -> null))
+ assert(outputEnvVars.toSet == expectedEnvVars.toSet)
+ }
+
+ test("SPARK-38582: verify that envVars built with field ref env as expected") {
+ val input = for (i <- 9 to 1 by -1) yield (s"testEnvKey.$i", s"v$i", "testEnvValue123456")
Review comment:
```suggestion
val input = for (i <- 9 to 1 by -1) yield (s"testEnvKey.$i", s"v$i", s"testEnvValue.$i")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dcoliversun edited a comment on pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVars and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
dcoliversun edited a comment on pull request #35886:
URL: https://github.com/apache/spark/pull/35886#issuecomment-1073530030
Gentle ping @dongjoon-hyun @martin-g
Could this PR be merged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] martin-g commented on a change in pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVarsWithKV and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #35886:
URL: https://github.com/apache/spark/pull/35886#discussion_r829749892
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -381,4 +381,42 @@ object KubernetesUtils extends Logging {
}
}
}
+
+ /**
+ * This function builds the EnvVar objects for each key-value env.
+ */
+ @Since("3.4.0")
+ def buildEnvVarsWithKV(env: Map[String, String]): Seq[EnvVar] = {
Review comment:
```suggestion
def buildEnvVars(env: Map[String, String]): Seq[EnvVar] = {
```
IMO there is no need of `WithKV` - the passed argument is not a Seq of tuples anymore.
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesUtilsSuite.scala
##########
@@ -127,4 +127,35 @@ class KubernetesUtilsSuite extends SparkFunSuite with PrivateMethodTester {
}
}
}
+
+ test("SPARK-38582: verify that envVars built with kv env as expected") {
+ val input = for (i <- 9 to 1 by -1) yield (s"testEnvKey.$i", "testEnvValue123456")
Review comment:
```suggestion
val input = for (i <- 9 to 1 by -1) yield (s"testEnvKey.$i", s"testEnvValue.$i")
```
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesUtilsSuite.scala
##########
@@ -127,4 +127,35 @@ class KubernetesUtilsSuite extends SparkFunSuite with PrivateMethodTester {
}
}
}
+
+ test("SPARK-38582: verify that envVars built with kv env as expected") {
+ val input = for (i <- 9 to 1 by -1) yield (s"testEnvKey.$i", "testEnvValue123456")
+ val expectedEnvVars = input.map { case(k, v) =>
+ new EnvVarBuilder()
+ .withName(k)
+ .withValue(v).build()
+ }
+ val outputEnvVars =
+ KubernetesUtils.buildEnvVarsWithKV(input.toMap + ("testKeyForNull" -> null))
+ assert(outputEnvVars.toSet == expectedEnvVars.toSet)
+ }
+
+ test("SPARK-38582: verify that envVars built with field ref env as expected") {
+ val input = for (i <- 9 to 1 by -1) yield (s"testEnvKey.$i", s"v$i", "testEnvValue123456")
Review comment:
```suggestion
val input = for (i <- 9 to 1 by -1) yield (s"testEnvKey.$i", s"v$i", s"testEnvValue.$i")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] martin-g commented on pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVars and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #35886:
URL: https://github.com/apache/spark/pull/35886#issuecomment-1073531480
I am just a contributor to the project. A project member could merge PRs.
// CC @dongjoon-hyun @HyukjinKwon @holdenk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dcoliversun commented on a change in pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVarsWithKV and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
dcoliversun commented on a change in pull request #35886:
URL: https://github.com/apache/spark/pull/35886#discussion_r829818581
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -381,4 +381,42 @@ object KubernetesUtils extends Logging {
}
}
}
+
+ /**
+ * This function builds the EnvVar objects for each key-value env.
+ */
+ @Since("3.4.0")
+ def buildEnvVarsWithKV(env: Map[String, String]): Seq[EnvVar] = {
Review comment:
Thanks for your comment :) Useful for me
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesUtilsSuite.scala
##########
@@ -127,4 +127,35 @@ class KubernetesUtilsSuite extends SparkFunSuite with PrivateMethodTester {
}
}
}
+
+ test("SPARK-38582: verify that envVars built with kv env as expected") {
+ val input = for (i <- 9 to 1 by -1) yield (s"testEnvKey.$i", "testEnvValue123456")
Review comment:
Nice. I fixed it~
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesUtilsSuite.scala
##########
@@ -127,4 +127,35 @@ class KubernetesUtilsSuite extends SparkFunSuite with PrivateMethodTester {
}
}
}
+
+ test("SPARK-38582: verify that envVars built with kv env as expected") {
+ val input = for (i <- 9 to 1 by -1) yield (s"testEnvKey.$i", "testEnvValue123456")
+ val expectedEnvVars = input.map { case(k, v) =>
+ new EnvVarBuilder()
+ .withName(k)
+ .withValue(v).build()
+ }
+ val outputEnvVars =
+ KubernetesUtils.buildEnvVarsWithKV(input.toMap + ("testKeyForNull" -> null))
+ assert(outputEnvVars.toSet == expectedEnvVars.toSet)
+ }
+
+ test("SPARK-38582: verify that envVars built with field ref env as expected") {
+ val input = for (i <- 9 to 1 by -1) yield (s"testEnvKey.$i", s"v$i", "testEnvValue123456")
Review comment:
Nice. I fixed it~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dcoliversun commented on pull request #35886: [SPARK-38582][K8S] Introduce buildEnvVarsWithKV and buildEnvVarsWithFieldRef for KubernetesUtils to eliminate duplicate code pattern
Posted by GitBox <gi...@apache.org>.
dcoliversun commented on pull request #35886:
URL: https://github.com/apache/spark/pull/35886#issuecomment-1072066622
@Yikun @martin-g
Update code for comments. It's good if you have a look again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org