Skip to content

Conversation

@aditanase
Copy link
Contributor

What changes were proposed in this pull request?

Adding docs for an enhancement that came in late in this PR: #22146
Currently the docs state that we're going to use the first container in a pod template, which was the implementation for some time, until it was improved with 2 new properties.

How was this patch tested?

I tested that the properties work by combining pod templates with client-mode and a simple pod template.

Please review http://spark.apache.org/contributing.html before opening a pull request.

@aditanase
Copy link
Contributor Author

cc @mccheah @onursatici @skonto

aditanase referenced this pull request Nov 27, 2018
…n docs.

## What changes were proposed in this pull request?

"Running on Kubernetes" references `spark.driver.pod.name` few places, and it should be `spark.kubernetes.driver.pod.name`.

## How was this patch tested?
See changes

Closes #23133 from Leemoonsoo/fix-driver-pod-name-prop.

Authored-by: Lee moon soo <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
@dongjoon-hyun
Copy link
Member

Thank you for your first contribution, @aditanase .

@dongjoon-hyun
Copy link
Member

ok to test

Copy link
Member

Choose a reason for hiding this comment

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

Could you move this to line 941 after spark.kubernetes.driver.podTemplateFile config? We had better group together spark.kubernetes.driver.* configs and spark.kubernetes.executor.* configs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done. I'll squash the commits before merging.

@SparkQA
Copy link

SparkQA commented Nov 27, 2018

Test build #99341 has finished for PR 23155 at commit 157edbc.

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

@SparkQA
Copy link

SparkQA commented Nov 27, 2018

@SparkQA
Copy link

SparkQA commented Nov 27, 2018

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

@SparkQA
Copy link

SparkQA commented Nov 28, 2018

Test build #99364 has finished for PR 23155 at commit 290b764.

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

@SparkQA
Copy link

SparkQA commented Nov 28, 2018

@SparkQA
Copy link

SparkQA commented Nov 28, 2018

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

@aditanase
Copy link
Contributor Author

@dongjoon-hyun is this ok to merge? Should I squash it and ping you again?

@dongjoon-hyun
Copy link
Member

Oh, I thought it could be handled by another Kubernetes committers. I'll take a look again.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, [pod template](#pod-template) is not working. Could you try the following and check with SKIP_API=1 jekyll build?

<a href="#pod-template">pod template</a>

Copy link
Member

Choose a reason for hiding this comment

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

Please fix this, too.

Copy link
Member

Choose a reason for hiding this comment

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

ditto.

Copy link
Member

Choose a reason for hiding this comment

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

ditto.

@srowen
Copy link
Member

srowen commented Dec 28, 2018

Ping @aditanase

@aditanase
Copy link
Contributor Author

@dongjoon-hyun Thanks for catching the broken anchors. Fixed them per your suggestion and tested with a jekyll build. Should work now.

@srowen my apologies, I've been on an extended winter break. I've rebased against master and squashed my commits - should be ready to merge.

@SparkQA
Copy link

SparkQA commented Jan 7, 2019

@SparkQA
Copy link

SparkQA commented Jan 7, 2019

Test build #100886 has finished for PR 23155 at commit 2f663f1.

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

@SparkQA
Copy link

SparkQA commented Jan 7, 2019

@SparkQA
Copy link

SparkQA commented Jan 7, 2019

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

@SparkQA
Copy link

SparkQA commented Jan 7, 2019

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

@srowen
Copy link
Member

srowen commented Jan 8, 2019

Merged to master

@srowen srowen closed this in 5fb5a02 Jan 8, 2019
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
## What changes were proposed in this pull request?

Adding docs for an enhancement that came in late in this PR: apache#22146
Currently the docs state that we're going to use the first container in a pod template, which was the implementation for some time, until it was improved with 2 new properties.

## How was this patch tested?

I tested that the properties work by combining pod templates with client-mode and a simple pod template.

Please review http://spark.apache.org/contributing.html before opening a pull request.

Closes apache#23155 from aditanase/k8s-readme.

Authored-by: Adrian Tanase <[email protected]>
Signed-off-by: Sean Owen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants