Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-22778][Kubernetes] Added the missing service metadata for KubernetesClusterManager #19972

Closed
wants to merge 3 commits into from

Conversation

liyinan926
Copy link
Contributor

@liyinan926 liyinan926 commented Dec 13, 2017

What changes were proposed in this pull request?

This PR added the missing service metadata for KubernetesClusterManager. Without the metadata, the service loader couldn't load KubernetesClusterManager, and caused the driver to fail to create a ExternalClusterManager, as being reported in SPARK-22778. The PR also changed the k8s: prefix used to k8s://, which is what existing Spark on k8s users are familiar and used to.

How was this patch tested?

Manual testing verified that the fix resolved the issue in SPARK-22778.

/cc @vanzin @felixcheung @jiangxb1987

@foxish
Copy link
Contributor

foxish commented Dec 13, 2017

LGTM.
Will independently verify the fix as well. Thanks @mccheah and @liyinan926 for debugging.

@liyinan926
Copy link
Contributor Author

@vanzin @felixcheung @jiangxb1987 Can one of you help take a look and merge? This is fixing a blocker issue for Kubernetes mode. Thanks!

@SparkQA
Copy link

SparkQA commented Dec 14, 2017

Test build #84878 has finished for PR 19972 at commit 48c4b0a.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@vanzin
Copy link
Contributor

vanzin commented Dec 14, 2017

One of the tests failures looks legitimate...

@liyinan926
Copy link
Contributor Author

@vanzin Yes, fixed in c91c9a6.

@SparkQA
Copy link

SparkQA commented Dec 14, 2017

Test build #84888 has finished for PR 19972 at commit c91c9a6.

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

@felixcheung
Copy link
Member

felixcheung commented Dec 14, 2017 via email

Copy link
Contributor

@jiangxb1987 jiangxb1987 left a comment

Choose a reason for hiding this comment

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

LGTM only one nit

@@ -0,0 +1 @@
org.apache.spark.scheduler.cluster.k8s.KubernetesClusterManager
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add an extra empty line below this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@SparkQA
Copy link

SparkQA commented Dec 14, 2017

Test build #84920 has finished for PR 19972 at commit a2e6396.

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

@vanzin
Copy link
Contributor

vanzin commented Dec 14, 2017

Merging to master.

@asfgit asfgit closed this in 2fe1633 Dec 14, 2017
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.

6 participants