[SPARK-34018][K8S] NPE in ExecutorPodsSnapshot

[holdenk]

What changes were proposed in this pull request?

Label both the statuses and ensure the ExecutorPodSnapshot starts with the default config to match.

Why are the changes needed?

The current test depends on the order rather than testing the desired property.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Labeled the containers statuses, observed failures, added the default label as the initialization point, tests passed again.

Built Spark, ran on K8s cluster verified no NPE in driver log.

[holdenk]

cc @dongjoon-hyun

[SparkQA]

Test build #133756 has finished for PR 31071 at commit 68a902f.

• 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/SparkPullRequestBuilder-K8s/38344/

[dongjoon-hyun]

Indentation?

[dongjoon-hyun]

If we have three case statements before this line, t is null and t is not null and exit code is non-zero and t is not null and exist code is zero` is covered. When we fall into here?

[holdenk]

We shouldn't but this is to prevent the spurious "non-exhaustive match" warning.

[dongjoon-hyun]

Oh, got it.

[dongjoon-hyun]

What about the following?

sparkContainerStatus.getState.getTerminated match {
  case t if t != null and t.getExitCode != 0 =>
    PodFailed(pod)
  case t if t != null and t.getExitCode == 0 =>
    PodSucceeded(pod)
  case _ =>
    PodRunning(pod)
}

[holdenk]

Sure that would work to. Do you prefer this one?

[dongjoon-hyun]

Yes, I do because it's shorter. :)

[holdenk]

I refactored this into using flatMaps so we can just pattern match on the result, WDYT?

[SparkQA]

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38344/

[holdenk]

Sounds good, I’ll change it over.

On Wed, Jan 6, 2021 at 9:19 PM Dongjoon Hyun ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsSnapshot.scala <#31071 (comment)>: > case t if t.getExitCode != 0 => PodFailed(pod) case t if t.getExitCode == 0 => PodSucceeded(pod) + // Fall through case case _ => PodRunning(pod) } Yes, I do because it's shorter. :) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31071 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAOT5PLDXUFED5M34LOVTLSYU75XANCNFSM4VX7W6RA> .

-- Cell : 425-233-8271

[SparkQA]

Test build #133804 has started for PR 31071 at commit c936501.

[dongjoon-hyun]

Please don't forget https://github.com/apache/spark/pull/31071/files#r552961918 .

[SparkQA]

Test build #133810 has finished for PR 31071 at commit 86b09a6.

• 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/SparkPullRequestBuilder-K8s/38399/

[SparkQA]

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38399/

[holdenk]

WDYT of the flatMap approach @dongjoon-hyun ?

[dongjoon-hyun]

+1, LGTM.

WDYT of the flatMap approach @dongjoon-hyun ?

The new approach looks nice! @holdenk