[SPARK-24694][K8S] Pass all app args to integration tests

[skonto]

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.

[SparkQA]

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/590/

[SparkQA]

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/590/

[SparkQA]

Test build #92483 has finished for PR 21672 at commit df59e04.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[skonto]

@ssuchter @liyinan926 psl review. This is trivial, unless I am missing something.

[ssuchter]

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.

[ssuchter]

I see why the old behavior was there. I made a minimal change to some existing code to fix a bug:

ssuchter@1d8a265

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?

[ssuchter]

BTW, for committers - I think this patch is good to merge.

[skonto]

@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.
I thought the same keep that condition or not, I guess we can remove it. Unless someone passes some null previously in the case class and even then that condition will throw an exception. It seems redundant that check unless we test against null values and if that is the case we create an empty array.

[vanzin]

Doesn't this make appArgsArray basically redundant?

[skonto]

Yes unless you pass a null... I updated my comment above ^^. Although I guess we take care of this earlier with an empty array whenever we call that method, so we should be safe. Anyway I can remove it.

[skonto]

@vanzin @ssuchter removed the condition, I think its ok now.

[SparkQA]

Test build #92489 has finished for PR 21672 at commit 92d8292.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/594/

[SparkQA]

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/testing-k8s-prb-make-spark-distribution-unified/594/

[felixcheung]

looks reasonable. is there a way to add test for this?

[skonto]

@felixcheung we could call a test job which accepts more than one args like: https://github.com/apache/spark/blob/d54d8b86301581142293341af25fd78b3278a2e8/examples/src/main/scala/org/apache/spark/examples/MultiBroadcastTest.scala.
Would that be ok? If so I can update the PR.
Other than that I think it does not make sense to have more tests for tests. This is test code we debug here anyway.

[skonto]

@felixcheung I think its ready to merge. In the future when we will add tests passing more params so we can verify it easily, but it is better than before.

[skonto]

@foxish @vanzin gentle ping.

[srowen]

Merged to master