Skip to content
This repository was archived by the owner on Jan 9, 2020. It is now read-only.

A number of small tweaks to the MVP. #23

Merged
merged 4 commits into from
Jan 24, 2017
Merged

A number of small tweaks to the MVP. #23

merged 4 commits into from
Jan 24, 2017

Conversation

@mccheah
Copy link

@mccheah mccheah commented Jan 17, 2017

  • Master protocol defaults to https if not specified
  • Removed upload driver extra classpath functionality
  • Added ability to specify main app resource with container:// URI
  • Shut down the REST server and thus the driver pod once the application completes
  • Updated docs to reflect all of the above
  • Add examples to Docker images, mostly for integration testing but could be useful for easily getting started without shipping anything to the cluster

@mccheah
A number of small tweaks to the MVP.
e1c27d5 
- Master protocol defaults to https if not specified
- Removed upload driver extra classpath functionality
- Added ability to specify main app resource with container:// URI
- Updated docs to reflect all of the above
- Add examples to Docker images, mostly for integration testing but
could be useful for easily getting started without shipping anything
@mccheah
Copy link
Author

mccheah commented Jan 17, 2017

@foxish @ash211 @erikerlandson bunch of small things here, though in retrospect each of these should be its own pull request.

ash211
ash211 reviewed Jan 17, 2017
View reviewed changes
docs/running-on-kubernetes.md Outdated
`spark.master` in the application's configuration, must be a URL with the format `k8s://<api_server_url>`. Prefixing the
master string with `k8s://` will cause the Spark application to launch on the Kubernetes cluster, with the API server
being contacted at `api_server_url`. The HTTP protocol must also be specified.
being contacted at `api_server_url`. If no HTTP protocol is specified in the URL, it defaults to `https`.
Copy link

@ash211 ash211 Jan 17, 2017

Choose a reason for hiding this comment

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

Provide an example of what this looks like. The double protocol thing is confusing as-is

Copy link
Author

@mccheah mccheah Jan 17, 2017

Choose a reason for hiding this comment

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

Done.

foxish
foxish reviewed Jan 18, 2017
View reviewed changes
.../core/src/main/scala/org/apache/spark/deploy/rest/kubernetes/KubernetesSparkRestServer.scala
sparkConf,
secretBytes)
secretBytes,
barrier)
Copy link
Member

@foxish foxish Jan 18, 2017

Choose a reason for hiding this comment

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

Why was barrier added?

Copy link
Author

@mccheah mccheah Jan 18, 2017

Choose a reason for hiding this comment

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

The barrier needs to count down to unblock the main thread whenever we stop the server.

@ash211
Copy link

ash211 commented Jan 19, 2017

@mccheah can you please handle merge conflicts?

@foxish anything else you'd want to see changed here before merging?

@foxish
Copy link
Member

foxish commented Jan 19, 2017

I still haven't gone over this change completely as I do not completely understand some parts of it. I'd like to go over it once tonight. Can we merge this tomorrow?

@foxish
Copy link
Member

foxish commented Jan 19, 2017

LGTM

@mccheah
Copy link
Author

mccheah commented Jan 24, 2017

@foxish @ash211 fixed merge conflicts. Integration tests were also failing from the pod and service name change, so fixed those also.

@mccheah mccheah mentioned this pull request Jan 24, 2017
Closed
ash211
ash211 approved these changes Jan 24, 2017
View reviewed changes
Copy link

@ash211 ash211 left a comment

Choose a reason for hiding this comment

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

@foxish @erikerlandson either of you have any concerns? Otherwise let's merge this

@erikerlandson
Copy link
Member

erikerlandson commented Jan 24, 2017

LGTM

@ash211 ash211 merged commit 46cda32 into k8s-support-alternate-incremental Jan 24, 2017
@ash211 ash211 deleted the small-tweaks branch January 24, 2017 02:02
@ash211 ash211 mentioned this pull request Jan 24, 2017
Closed
ash211 pushed a commit that referenced this pull request Feb 8, 2017
@mccheah @ash211
A number of small tweaks to the MVP. (#23)
4f6f42f 
* A number of small tweaks to the MVP.

- Master protocol defaults to https if not specified
- Removed upload driver extra classpath functionality
- Added ability to specify main app resource with container:// URI
- Updated docs to reflect all of the above
- Add examples to Docker images, mostly for integration testing but
could be useful for easily getting started without shipping anything

* Add example to documentation.
ash211 pushed a commit that referenced this pull request Mar 8, 2017
@mccheah @ash211
A number of small tweaks to the MVP. (#23)
3fd9c62 
* A number of small tweaks to the MVP.

- Master protocol defaults to https if not specified
- Removed upload driver extra classpath functionality
- Added ability to specify main app resource with container:// URI
- Updated docs to reflect all of the above
- Add examples to Docker images, mostly for integration testing but
could be useful for easily getting started without shipping anything

* Add example to documentation.
foxish pushed a commit that referenced this pull request Jul 24, 2017
@mccheah @foxish
A number of small tweaks to the MVP. (#23)
95747bc 
* A number of small tweaks to the MVP.

- Master protocol defaults to https if not specified
- Removed upload driver extra classpath functionality
- Added ability to specify main app resource with container:// URI
- Updated docs to reflect all of the above
- Add examples to Docker images, mostly for integration testing but
could be useful for easily getting started without shipping anything

* Add example to documentation.
ifilonenko pushed a commit to ifilonenko/spark that referenced this pull request Feb 25, 2019
@mccheah @ash211
A number of small tweaks to the MVP. (apache-spark-on-k8s#23)
f142ab1 
* A number of small tweaks to the MVP.

- Master protocol defaults to https if not specified
- Removed upload driver extra classpath functionality
- Added ability to specify main app resource with container:// URI
- Updated docs to reflect all of the above
- Add examples to Docker images, mostly for integration testing but
could be useful for easily getting started without shipping anything

* Add example to documentation.
puneetloya pushed a commit to puneetloya/spark that referenced this pull request Mar 11, 2019
@mccheah
A number of small tweaks to the MVP. (apache-spark-on-k8s#23)
1b55e80 
* A number of small tweaks to the MVP.

- Master protocol defaults to https if not specified
- Removed upload driver extra classpath functionality
- Added ability to specify main app resource with container:// URI
- Updated docs to reflect all of the above
- Add examples to Docker images, mostly for integration testing but
could be useful for easily getting started without shipping anything

* Add example to documentation.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@foxish foxish foxish left review comments

@ash211 ash211 ash211 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mccheah @ash211 @foxish @erikerlandson