Skip to content

Conversation

@zwangsheng
Copy link
Contributor

@zwangsheng zwangsheng commented Mar 14, 2023

What changes were proposed in this pull request?

After #37880 when user spark submit without --deploy-mode XXX or –conf spark.submit.deployMode=XXXX, may face NPE with this code.

Why are the changes needed?

https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala#164

args.deployMode.equals("client") &&

Of course, submit without deployMode is not allowed and will throw an exception and terminate the application, but we should leave it to the later logic to give the appropriate hint instead of giving a NPE.

Does this PR introduce any user-facing change?

No

How was this patch tested?

popo_2023-03-14  17-50-46

@github-actions github-actions bot added the CORE label Mar 14, 2023
@zwangsheng
Copy link
Contributor Author

As last PR mentioned, @yaooqinn @shrprasa

@yaooqinn
Copy link
Member

yaooqinn commented Mar 14, 2023

Please use the jira-style prefix [SPARK-42785], not a GitHub issue-like one [SPARK #42785] like apache/kyuubi. Please also answer the rest of the questions in the PR description.

@shrprasa
Copy link
Contributor

@zwangsheng @yaooqinn There won't be any NPE if you try to actually submit a job without the deploy-mode because if no value is provided, it defaults to "client". Please refer to org.apache.spark.internal.config/package.scala.

  private[spark] val SUBMIT_DEPLOY_MODE = ConfigBuilder("spark.submit.deployMode")
    .version("1.5.0")
    .stringConf
    .createWithDefault("client")

@zwangsheng
Copy link
Contributor Author

zwangsheng commented Mar 14, 2023

@zwangsheng @yaooqinn There won't be any NPE if you try to actually submit a job without the deploy-mode because if no value is provided, it defaults to "client". Please refer to org.apache.spark.internal.config/package.scala.

  private[spark] val SUBMIT_DEPLOY_MODE = ConfigBuilder("spark.submit.deployMode")
    .version("1.5.0")
    .stringConf
    .createWithDefault("client")

@shrprasa Thanks for reminder

I saw this default value, you can do local test with writing following code
popo_2023-03-14  17-50-46

The default value client you mean, will be set in prepareSubmitEnvironment function, which be called in runMain() after check

args.deployMode.equals("client") &&

@zwangsheng zwangsheng changed the title [SPARK #42785][K8S][Core] When spark submit without --deploy-mode, avoid facing NPE in Kubernetes Case [SPARK-42785][K8S][Core] When spark submit without --deploy-mode, avoid facing NPE in Kubernetes Case Mar 14, 2023
@zwangsheng
Copy link
Contributor Author

Please use the jira-style prefix [SPARK-42785], not a GitHub issue-like one [SPARK #42785] like apache/kyuubi

Sure. XD

@shrprasa
Copy link
Contributor

I was able to reproduce the issue. Thanks @zwangsheng for fixing it.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM. Thank you, @zwangsheng and all.
Merged to master/3.4/3.3/3.2.

dongjoon-hyun pushed a commit that referenced this pull request Mar 14, 2023
…void facing NPE in Kubernetes Case

### What changes were proposed in this pull request?

After #37880 when user spark submit without `--deploy-mode XXX` or `–conf spark.submit.deployMode=XXXX`, may face NPE with this code.

### Why are the changes needed?

https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala#164
```scala
args.deployMode.equals("client") &&
```

Of course, submit without `deployMode` is not allowed and will throw an exception and terminate the application, but we should leave it to the later logic to give the appropriate hint instead of giving a NPE.

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

![popo_2023-03-14  17-50-46](https://user-images.githubusercontent.com/52876270/224965310-ba9ec82f-e668-4a06-b6ff-34c3e80ca0b4.jpg)

Closes #40414 from zwangsheng/SPARK-42785.

Authored-by: zwangsheng <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 767253b)
Signed-off-by: Dongjoon Hyun <[email protected]>
dongjoon-hyun pushed a commit that referenced this pull request Mar 14, 2023
…void facing NPE in Kubernetes Case

### What changes were proposed in this pull request?

After #37880 when user spark submit without `--deploy-mode XXX` or `–conf spark.submit.deployMode=XXXX`, may face NPE with this code.

### Why are the changes needed?

https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala#164
```scala
args.deployMode.equals("client") &&
```

Of course, submit without `deployMode` is not allowed and will throw an exception and terminate the application, but we should leave it to the later logic to give the appropriate hint instead of giving a NPE.

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

![popo_2023-03-14  17-50-46](https://user-images.githubusercontent.com/52876270/224965310-ba9ec82f-e668-4a06-b6ff-34c3e80ca0b4.jpg)

Closes #40414 from zwangsheng/SPARK-42785.

Authored-by: zwangsheng <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 767253b)
Signed-off-by: Dongjoon Hyun <[email protected]>
dongjoon-hyun pushed a commit that referenced this pull request Mar 14, 2023
…void facing NPE in Kubernetes Case

### What changes were proposed in this pull request?

After #37880 when user spark submit without `--deploy-mode XXX` or `–conf spark.submit.deployMode=XXXX`, may face NPE with this code.

### Why are the changes needed?

https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala#164
```scala
args.deployMode.equals("client") &&
```

Of course, submit without `deployMode` is not allowed and will throw an exception and terminate the application, but we should leave it to the later logic to give the appropriate hint instead of giving a NPE.

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

![popo_2023-03-14  17-50-46](https://user-images.githubusercontent.com/52876270/224965310-ba9ec82f-e668-4a06-b6ff-34c3e80ca0b4.jpg)

Closes #40414 from zwangsheng/SPARK-42785.

Authored-by: zwangsheng <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 767253b)
Signed-off-by: Dongjoon Hyun <[email protected]>
sunchao pushed a commit to sunchao/spark that referenced this pull request Jun 2, 2023
…void facing NPE in Kubernetes Case

### What changes were proposed in this pull request?

After apache#37880 when user spark submit without `--deploy-mode XXX` or `–conf spark.submit.deployMode=XXXX`, may face NPE with this code.

### Why are the changes needed?

https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala#164
```scala
args.deployMode.equals("client") &&
```

Of course, submit without `deployMode` is not allowed and will throw an exception and terminate the application, but we should leave it to the later logic to give the appropriate hint instead of giving a NPE.

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

![popo_2023-03-14  17-50-46](https://user-images.githubusercontent.com/52876270/224965310-ba9ec82f-e668-4a06-b6ff-34c3e80ca0b4.jpg)

Closes apache#40414 from zwangsheng/SPARK-42785.

Authored-by: zwangsheng <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 767253b)
Signed-off-by: Dongjoon Hyun <[email protected]>
snmvaughan pushed a commit to snmvaughan/spark that referenced this pull request Jun 20, 2023
…void facing NPE in Kubernetes Case

### What changes were proposed in this pull request?

After apache#37880 when user spark submit without `--deploy-mode XXX` or `–conf spark.submit.deployMode=XXXX`, may face NPE with this code.

### Why are the changes needed?

https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala#164
```scala
args.deployMode.equals("client") &&
```

Of course, submit without `deployMode` is not allowed and will throw an exception and terminate the application, but we should leave it to the later logic to give the appropriate hint instead of giving a NPE.

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

![popo_2023-03-14  17-50-46](https://user-images.githubusercontent.com/52876270/224965310-ba9ec82f-e668-4a06-b6ff-34c3e80ca0b4.jpg)

Closes apache#40414 from zwangsheng/SPARK-42785.

Authored-by: zwangsheng <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 767253b)
Signed-off-by: Dongjoon Hyun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants