You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by skonto <gi...@git.apache.org> on 2018/06/29 16:39:19 UTC
[GitHub] spark pull request #21672: [SPARK-24694][K8S] Pass all app args to integrati...
GitHub user skonto opened a pull request:
https://github.com/apache/spark/pull/21672
[SPARK-24694][K8S] Pass all app args to integration tests
## What changes were proposed in this pull request?
- Allows to pass more than one app args to tests.
## How was this patch tested?
Manually tested it with a spark test that requires more than on app args.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/skonto/spark fix_itsets-args
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/21672.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #21672
----
commit df59e04aaa4e2c6de4d204cd63a9fa753e2fb687
Author: Stavros Kontopoulos <st...@...>
Date: 2018-06-29T16:36:39Z
minor fix itests
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/21672
Merged to master
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21672
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/594/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21672
**[Test build #92483 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92483/testReport)** for PR 21672 at commit [`df59e04`](https://github.com/apache/spark/commit/df59e04aaa4e2c6de4d204cd63a9fa753e2fb687).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21672
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/92483/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21672: [SPARK-24694][K8S] Pass all app args to integrati...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/21672
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on the issue:
https://github.com/apache/spark/pull/21672
@felixcheung we could call a test with more than one parameters like: https://github.com/apache/spark/blob/master/examples/src/main/scala/org/apache/spark/examples/MultiBroadcastTest.scala
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21672: [SPARK-24694][K8S] Pass all app args to integrati...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21672#discussion_r199297547
--- Diff: resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/KubernetesTestComponents.scala ---
@@ -106,7 +106,7 @@ private[spark] object SparkAppLauncher extends Logging {
val sparkSubmitExecutable = sparkHomeDir.resolve(Paths.get("bin", "spark-submit"))
logInfo(s"Launching a spark app with arguments $appArguments and conf $appConf")
val appArgsArray =
- if (appArguments.appArgs.length > 0) Array(appArguments.appArgs.mkString(" "))
--- End diff --
Doesn't this make `appArgsArray` basically redundant?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21672
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/92489/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21672
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/590/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21672
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21672
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/590/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by ssuchter <gi...@git.apache.org>.
Github user ssuchter commented on the issue:
https://github.com/apache/spark/pull/21672
I see why the old behavior was there. I made a minimal change to some existing code to fix a bug:
https://github.com/ssuchter/spark/commit/1d8a265d13b65dcec8db11a5be09d4a029037d2c
but this new way is better.
Question: In this new way, do we even need the test for appArguments.appArgs.length > 0? Could we just use appArguments.appArgs?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21672
**[Test build #92489 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92489/testReport)** for PR 21672 at commit [`92d8292`](https://github.com/apache/spark/commit/92d8292deed6de8e160fdb82adbcd4e9d5d00a48).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by ssuchter <gi...@git.apache.org>.
Github user ssuchter commented on the issue:
https://github.com/apache/spark/pull/21672
So this changes behavior, I think. In the old behavior, if the args were ['a', 'b'] then you'd get a single arg of ['a b'] passed through, and with this you'd get ['a', 'b'].
This new behavior seems better, I'm just trying a bit to remember why we had the old behavior.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21672
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21672
**[Test build #92489 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92489/testReport)** for PR 21672 at commit [`92d8292`](https://github.com/apache/spark/commit/92d8292deed6de8e160fdb82adbcd4e9d5d00a48).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on the issue:
https://github.com/apache/spark/pull/21672
@felixcheung I think its ok to merge. In the future when we will add tests passing more params we can verify it easily, but it is better than before.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21672
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21672
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/594/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21672
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/594/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on the issue:
https://github.com/apache/spark/pull/21672
@ssuchter @liyinan926 psl review. This is trivial, unless I am missing something.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21672
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/590/
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on the issue:
https://github.com/apache/spark/pull/21672
@ssuchter yes if you check the jira, the old behavior does not work with more than one args. In the future might be a problem.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on the issue:
https://github.com/apache/spark/pull/21672
@vanzin @ssuchter removed the condition, I think its ok now.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21672
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by ssuchter <gi...@git.apache.org>.
Github user ssuchter commented on the issue:
https://github.com/apache/spark/pull/21672
BTW, for committers - I think this patch is good to merge.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on the issue:
https://github.com/apache/spark/pull/21672
@foxish @srowen gentle ping.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21672: [SPARK-24694][K8S] Pass all app args to integrati...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on a diff in the pull request:
https://github.com/apache/spark/pull/21672#discussion_r199297784
--- Diff: resource-managers/kubernetes/integration-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/KubernetesTestComponents.scala ---
@@ -106,7 +106,7 @@ private[spark] object SparkAppLauncher extends Logging {
val sparkSubmitExecutable = sparkHomeDir.resolve(Paths.get("bin", "spark-submit"))
logInfo(s"Launching a spark app with arguments $appArguments and conf $appConf")
val appArgsArray =
- if (appArguments.appArgs.length > 0) Array(appArguments.appArgs.mkString(" "))
--- End diff --
Yes unless you pass a null... I updated my comment above ^^.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21672: [SPARK-24694][K8S] Pass all app args to integration test...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21672
**[Test build #92483 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92483/testReport)** for PR 21672 at commit [`df59e04`](https://github.com/apache/spark/commit/df59e04aaa4e2c6de4d204cd63a9fa753e2fb687).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org