You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by GitBox <gi...@apache.org> on 2022/11/24 13:03:00 UTC
[GitHub] [incubator-kyuubi] xuefeimiaoao opened a new pull request, #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
xuefeimiaoao opened a new pull request, #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852
…bernetes environment
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/CONTRIBUTING.html
2. If the PR is related to an issue in https://github.com/apache/incubator-kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
-->
### _Why are the changes needed?_
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about the use case of it.
2. If you fix a bug, you can clarify why it is a bug.
-->
We can deploy spark on kubernetes without configure spark.master explicitly when api-server url is not exposed for us.
### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
we do not set `spark.master`:
kyuubi add `spark.master` when setting `kyuubi.kubernetes.engine.master.override` to true.
- [ ] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] xuefeimiaoao commented on a diff in pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
xuefeimiaoao commented on code in PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#discussion_r1032739949
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -53,8 +55,30 @@ class SparkProcessBuilder(
Paths.get(sparkHome, "bin", SPARK_SUBMIT_FILE).toFile.getCanonicalPath
}
+ initialize
+
override def mainClass: String = "org.apache.kyuubi.engine.spark.SparkSQLEngine"
+ /**
+ * Add or override spark.master if KUBERNETES_SERVICE_HOST and
+ * KUBERNETES_SERVICE_PORT are defined
+ */
+ protected def initialize(): Unit = {
+ try {
+ if (conf.get(KUBERNETES_MASTER_OVERRIDE)) {
Review Comment:
I agree with your view and complete `spark.master` only in case of its absence.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#discussion_r1032161311
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -53,8 +55,30 @@ class SparkProcessBuilder(
Paths.get(sparkHome, "bin", SPARK_SUBMIT_FILE).toFile.getCanonicalPath
}
+ initialize
+
override def mainClass: String = "org.apache.kyuubi.engine.spark.SparkSQLEngine"
+ /**
+ * Add or override spark.master if KUBERNETES_SERVICE_HOST and
+ * KUBERNETES_SERVICE_PORT are defined
+ */
+ protected def initialize(): Unit = {
+ try {
+ if (conf.get(KUBERNETES_MASTER_OVERRIDE)) {
+ val kubernetesServiceHost = sys.env.get(KUBERNETES_SERVICE_HOST)
+ val kubernetesServicePort = sys.env.get(KUBERNETES_SERVICE_PORT)
+ if (kubernetesServiceHost.isDefined && kubernetesServicePort.isDefined) {
+ val masterURL = s"k8s://https://${kubernetesServiceHost.get}:${kubernetesServicePort.get}"
Review Comment:
w/ hardcoded "https", how to support "http"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on pull request #3852: [KYUUBI #3851][SPARK] Support auto set up `spark.master` when Kyuubi running inside Pod
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1330039967
@xuefeimiaoao I in favor the previous idea, we don't need to check the deploy mode. let's wait @zwangsheng to fix the `connectionConf` first
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on pull request #3852: [KYUUBI #3851][SPARK] Support auto set up `spark.master` when Kyuubi running inside Pod
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1330028338
> ... skipping setting spark.master when both spark.master and spark.submit.deployMode are not set.
sounds very limit.
> I am confused how `org.apache.kyuubi.kubernetes.test.deployment.KyuubiOnKubernetesWithLocalSparkTestsSuite#connectionConf` passed to kyuubi server when testing
It set `"spark.master" -> "local"`, should not be affected?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] xuefeimiaoao commented on a diff in pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
xuefeimiaoao commented on code in PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#discussion_r1033077245
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -53,8 +54,34 @@ class SparkProcessBuilder(
Paths.get(sparkHome, "bin", SPARK_SUBMIT_FILE).toFile.getCanonicalPath
}
+ completeMasterUrl()
+
override def mainClass: String = "org.apache.kyuubi.engine.spark.SparkSQLEngine"
+ /**
+ * Add `spark.master` if KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT
+ * are defined. So we can deploy spark on kubernetes without setting `spark.master`
+ * explicitly when kyuubi-servers are on kubernetes, which also helps in case that
+ * api-server is not exposed to us.
+ */
+ protected def completeMasterUrl(): Unit = {
+ try {
+ val kubernetesServiceHost = sys.env.get(KUBERNETES_SERVICE_HOST)
+ val kubernetesServicePort = sys.env.get(KUBERNETES_SERVICE_PORT)
+ if (conf.getOption(MASTER_KEY).isEmpty
Review Comment:
Besides, i found that spark.master will be lost if which is set under SPARK_CONF_DIR, which seems like a bug, so i add some code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] zwangsheng commented on pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
zwangsheng commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1328486923
In the case you mentioned that the users should not aware of the kubernetes api service address, should we consider config `spark.kubernetes.driver.master` for users in cluster mode?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1327202078
Thanks @xuefeimiaoao, it's good to make Kyuubi smart and out-of-box for Spark on K8s, left some comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#discussion_r1033081656
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -53,8 +54,42 @@ class SparkProcessBuilder(
Paths.get(sparkHome, "bin", SPARK_SUBMIT_FILE).toFile.getCanonicalPath
}
+ completeMasterUrl()
+
override def mainClass: String = "org.apache.kyuubi.engine.spark.SparkSQLEngine"
+ /**
+ * Add `spark.master` if KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT
+ * are defined. So we can deploy spark on kubernetes without setting `spark.master`
+ * explicitly when kyuubi-servers are on kubernetes, which also helps in case that
+ * api-server is not exposed to us.
+ * Besides, set `spark.master` into kyuubiConf if which is set in spark-defaults.conf
+ * under SPARK_CONF_DIR.
+ */
+ protected def completeMasterUrl(): Unit = {
+ try {
+ (
+ clusterManager(),
+ sys.env.get(KUBERNETES_SERVICE_HOST),
+ sys.env.get(KUBERNETES_SERVICE_PORT)) match {
+ case (None, Some(kubernetesServiceHost), Some(kubernetesServicePort)) =>
+ // According to "https://kubernetes.io/docs/concepts/architecture/control-plane-
+ // node-communication/#node-to-control-plane", the API server is configured to listen
+ // for remote connections on a secure HTTPS port (typically 443), so we set https here.
+ val masterURL = s"k8s://https://${kubernetesServiceHost}:${kubernetesServicePort}"
+ conf.set(MASTER_KEY, masterURL)
+ case (Some(clusterManager), _, _) =>
+ // in case that ClusterManager was set in spark-defaults.conf, which should be set into
+ // KyuubiConf, the one which is used to build sparkSubmit.
+ conf.setIfMissing(MASTER_KEY, clusterManager)
Review Comment:
it's redundant, please remove it and keep thing simple.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] xuefeimiaoao commented on pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
xuefeimiaoao commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1328572639
> In the case you mentioned that the users should not aware of the kubernetes api service address, should we consider config `spark.kubernetes.driver.master` for users in cluster mode?
@zwangsheng
I suppose that we could respect user defined config, whether it is `spark.master` or `spark.kubernetes.driver.master`, nonetheless, we actually do not need those configs in most cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] xuefeimiaoao commented on a diff in pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
xuefeimiaoao commented on code in PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#discussion_r1032740100
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -53,8 +55,30 @@ class SparkProcessBuilder(
Paths.get(sparkHome, "bin", SPARK_SUBMIT_FILE).toFile.getCanonicalPath
}
+ initialize
+
override def mainClass: String = "org.apache.kyuubi.engine.spark.SparkSQLEngine"
+ /**
+ * Add or override spark.master if KUBERNETES_SERVICE_HOST and
+ * KUBERNETES_SERVICE_PORT are defined
+ */
+ protected def initialize(): Unit = {
+ try {
+ if (conf.get(KUBERNETES_MASTER_OVERRIDE)) {
+ val kubernetesServiceHost = sys.env.get(KUBERNETES_SERVICE_HOST)
+ val kubernetesServicePort = sys.env.get(KUBERNETES_SERVICE_PORT)
+ if (kubernetesServiceHost.isDefined && kubernetesServicePort.isDefined) {
+ val masterURL = s"k8s://https://${kubernetesServiceHost.get}:${kubernetesServicePort.get}"
Review Comment:
After checking "https://kubernetes.io/docs/concepts/architecture/control-plane-node-communication/#node-to-control-plane", the API server is configured to listen for remote connections on a secure HTTPS port (typically 443), so just set https here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] xuefeimiaoao commented on pull request #3852: [KYUUBI #3851][SPARK] Support auto set up `spark.master` when Kyuubi running inside Pod
Posted by GitBox <gi...@apache.org>.
xuefeimiaoao commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1330014549
>
I suppose that spark will use default master `local[*]` encountering missing `spark.master` according to `org.apache.spark.deploy.SparkSubmitArguments#loadEnvironmentArguments`, which incurs a problem of `service account doesn't have access` cus serviceaccount was not set, so that we should skipping setting `spark.master` when both `spark.master` and `spark.submit.deployMode` are not set. In another word, we will just auto-set `spark.master` when `spark.master` is not set and `spark.submit.deployMode` is set explicitly.@pan3793
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] xuefeimiaoao commented on pull request #3852: [KYUUBI #3851][SPARK] Support auto set up `spark.master` when Kyuubi running inside Pod
Posted by GitBox <gi...@apache.org>.
xuefeimiaoao commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1330041113
> @xuefeimiaoao would mind sending a simple pr to unlock the ci? e.g. #3861 once a pr get merged, you don't need to wait for approve to run ci
With pleasure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] xuefeimiaoao commented on a diff in pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
xuefeimiaoao commented on code in PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#discussion_r1033073971
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -53,8 +54,34 @@ class SparkProcessBuilder(
Paths.get(sparkHome, "bin", SPARK_SUBMIT_FILE).toFile.getCanonicalPath
}
+ completeMasterUrl()
+
override def mainClass: String = "org.apache.kyuubi.engine.spark.SparkSQLEngine"
+ /**
+ * Add `spark.master` if KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT
+ * are defined. So we can deploy spark on kubernetes without setting `spark.master`
+ * explicitly when kyuubi-servers are on kubernetes, which also helps in case that
+ * api-server is not exposed to us.
+ */
+ protected def completeMasterUrl(): Unit = {
+ try {
+ val kubernetesServiceHost = sys.env.get(KUBERNETES_SERVICE_HOST)
+ val kubernetesServicePort = sys.env.get(KUBERNETES_SERVICE_PORT)
+ if (conf.getOption(MASTER_KEY).isEmpty
Review Comment:
` (
clusterManager(),
sys.env.get(KUBERNETES_SERVICE_HOST),
sys.env.get(KUBERNETES_SERVICE_PORT)) match {
case (None, Some(kubernetesServiceHost), Some(kubernetesServicePort)) =>
// According to "https://kubernetes.io/docs/concepts/architecture/control-plane-
// node-communication/#node-to-control-plane", the API server is configured to listen
// for remote connections on a secure HTTPS port (typically 443), so we set https here.
val masterURL = s"k8s://https://${kubernetesServiceHost}:${kubernetesServicePort}"
conf.set(MASTER_KEY, masterURL)
case (Some(clusterManager), _, _) =>
// in case that ClusterManager was set in spark-defaults.conf, which should be set into
// KyuubiConf, the one which is used to build sparkSubmit.
conf.setIfMissing(MASTER_KEY, clusterManager)
case _ =>
}`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] zwangsheng commented on pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
zwangsheng commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1328576304
> I suppose that we could respect user defined config, whether it is `spark.master` or `spark.kubernetes.driver.master`, nonetheless, we actually do not need those configs in most cases.
I understand what you said and this pr LGTM. I mean in the case we mentioned that we need to help the users set up `spark.master` (Users can't sense kubernetes master and commit using cluster deploy mode), do we need to help the user configure this `spark.kubernetes.driver.master` at the same time?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 closed pull request #3852: [KYUUBI #3851][SPARK] Support auto set up `spark.master` when Kyuubi running inside Pod
Posted by GitBox <gi...@apache.org>.
pan3793 closed pull request #3852: [KYUUBI #3851][SPARK] Support auto set up `spark.master` when Kyuubi running inside Pod
URL: https://github.com/apache/kyuubi/pull/3852
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] xuefeimiaoao commented on a diff in pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
xuefeimiaoao commented on code in PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#discussion_r1032741425
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -53,8 +55,30 @@ class SparkProcessBuilder(
Paths.get(sparkHome, "bin", SPARK_SUBMIT_FILE).toFile.getCanonicalPath
}
+ initialize
+
override def mainClass: String = "org.apache.kyuubi.engine.spark.SparkSQLEngine"
+ /**
+ * Add or override spark.master if KUBERNETES_SERVICE_HOST and
+ * KUBERNETES_SERVICE_PORT are defined
+ */
+ protected def initialize(): Unit = {
+ try {
+ if (conf.get(KUBERNETES_MASTER_OVERRIDE)) {
+ val kubernetesServiceHost = sys.env.get(KUBERNETES_SERVICE_HOST)
+ val kubernetesServicePort = sys.env.get(KUBERNETES_SERVICE_PORT)
+ if (kubernetesServiceHost.isDefined && kubernetesServicePort.isDefined) {
+ val masterURL = s"k8s://https://${kubernetesServiceHost.get}:${kubernetesServicePort.get}"
Review Comment:
If user want to use http, he could set `spark.master` explicitly, which we will respect and will not be overridden after changing of my code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#discussion_r1032931392
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -53,8 +54,34 @@ class SparkProcessBuilder(
Paths.get(sparkHome, "bin", SPARK_SUBMIT_FILE).toFile.getCanonicalPath
}
+ completeMasterUrl()
+
override def mainClass: String = "org.apache.kyuubi.engine.spark.SparkSQLEngine"
+ /**
+ * Add `spark.master` if KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT
+ * are defined. So we can deploy spark on kubernetes without setting `spark.master`
+ * explicitly when kyuubi-servers are on kubernetes, which also helps in case that
+ * api-server is not exposed to us.
+ */
+ protected def completeMasterUrl(): Unit = {
+ try {
+ val kubernetesServiceHost = sys.env.get(KUBERNETES_SERVICE_HOST)
+ val kubernetesServicePort = sys.env.get(KUBERNETES_SERVICE_PORT)
+ if (conf.getOption(MASTER_KEY).isEmpty
Review Comment:
And the code can be simplified to
```
(clusterManager(), sys.env.get(KUBERNETES_SERVICE_HOST), sys.env.get(KUBERNETES_SERVICE_PORT)) match {
case (None, Some(kubernetesServiceHost), Some(kubernetesServicePort)) =>
...
case _ =>
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] zwangsheng commented on pull request #3852: [KYUUBI #3851][SPARK] Support auto set up `spark.master` when Kyuubi running inside Pod
Posted by GitBox <gi...@apache.org>.
zwangsheng commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1330037379
> I am confused how `org.apache.kyuubi.kubernetes.test.deployment.KyuubiOnKubernetesWithLocalSparkTestsSuite#connectionConf` passed to kyuubi server when testing. @zwangsheng
It seems we are not using this connect conf, I'll turn on a pr to fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] xuefeimiaoao commented on pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
xuefeimiaoao commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1328589278
CI has been blocked by test of DiscoveryClientTests, could you help to skip it or trigger recheck again?@zwangsheng
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] xuefeimiaoao commented on pull request #3852: [KYUUBI #3851][SPARK] Support auto set up `spark.master` when Kyuubi running inside Pod
Posted by GitBox <gi...@apache.org>.
xuefeimiaoao commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1330038366
> > ... skipping setting spark.master when both spark.master and spark.submit.deployMode are not set.
>
> sounds very limit.
Indeed, and could you help me review and retrigger ci again
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on pull request #3852: [KYUUBI #3851][SPARK] Support auto set up `spark.master` when Kyuubi running inside Pod
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1330039358
@xuefeimiaoao would mind send a simple pr to unlock the ci? e.g. https://github.com/apache/incubator-kyuubi/issues/3861 once a pr got merge, you don't need to approve to run ci
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] zwangsheng commented on pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
zwangsheng commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1328593478
> CI has been blocked by test of DiscoveryClientTests, could you help to skip it or trigger recheck again?@zwangsheng
@pan3793 ,ping.
Need retry CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] zwangsheng commented on pull request #3852: [KYUUBI #3851][SPARK] Support auto set up `spark.master` when Kyuubi running inside Pod
Posted by GitBox <gi...@apache.org>.
zwangsheng commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1337040044
Hi @xuefeimiaoao, sorry for late fix lt test.
Kyuubi On Kubernetes IT Test will be fixed in this #3869 PR, and your can rebase this pr after it's meraged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on pull request #3852: [KYUUBI #3851][SPARK] Support auto set up `spark.master` when Kyuubi running inside Pod
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3852:
URL: https://github.com/apache/kyuubi/pull/3852#issuecomment-1370885534
Thanks, merged to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#discussion_r1032930862
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -53,8 +54,34 @@ class SparkProcessBuilder(
Paths.get(sparkHome, "bin", SPARK_SUBMIT_FILE).toFile.getCanonicalPath
}
+ completeMasterUrl()
+
override def mainClass: String = "org.apache.kyuubi.engine.spark.SparkSQLEngine"
+ /**
+ * Add `spark.master` if KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT
+ * are defined. So we can deploy spark on kubernetes without setting `spark.master`
+ * explicitly when kyuubi-servers are on kubernetes, which also helps in case that
+ * api-server is not exposed to us.
+ */
+ protected def completeMasterUrl(): Unit = {
+ try {
+ val kubernetesServiceHost = sys.env.get(KUBERNETES_SERVICE_HOST)
+ val kubernetesServicePort = sys.env.get(KUBERNETES_SERVICE_PORT)
+ if (conf.getOption(MASTER_KEY).isEmpty
Review Comment:
I think `clusterManager()` should be called instead
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] xuefeimiaoao commented on a diff in pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
xuefeimiaoao commented on code in PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#discussion_r1033076368
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -53,8 +54,34 @@ class SparkProcessBuilder(
Paths.get(sparkHome, "bin", SPARK_SUBMIT_FILE).toFile.getCanonicalPath
}
+ completeMasterUrl()
+
override def mainClass: String = "org.apache.kyuubi.engine.spark.SparkSQLEngine"
+ /**
+ * Add `spark.master` if KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT
+ * are defined. So we can deploy spark on kubernetes without setting `spark.master`
+ * explicitly when kyuubi-servers are on kubernetes, which also helps in case that
+ * api-server is not exposed to us.
+ */
+ protected def completeMasterUrl(): Unit = {
+ try {
+ val kubernetesServiceHost = sys.env.get(KUBERNETES_SERVICE_HOST)
+ val kubernetesServicePort = sys.env.get(KUBERNETES_SERVICE_PORT)
+ if (conf.getOption(MASTER_KEY).isEmpty
Review Comment:
I think `clusterManager()` is better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#discussion_r1032160219
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -53,8 +55,30 @@ class SparkProcessBuilder(
Paths.get(sparkHome, "bin", SPARK_SUBMIT_FILE).toFile.getCanonicalPath
}
+ initialize
+
override def mainClass: String = "org.apache.kyuubi.engine.spark.SparkSQLEngine"
+ /**
+ * Add or override spark.master if KUBERNETES_SERVICE_HOST and
+ * KUBERNETES_SERVICE_PORT are defined
+ */
+ protected def initialize(): Unit = {
+ try {
+ if (conf.get(KUBERNETES_MASTER_OVERRIDE)) {
Review Comment:
how about try auto configure it iff `spark.master` is absent?
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -53,8 +55,30 @@ class SparkProcessBuilder(
Paths.get(sparkHome, "bin", SPARK_SUBMIT_FILE).toFile.getCanonicalPath
}
+ initialize
+
override def mainClass: String = "org.apache.kyuubi.engine.spark.SparkSQLEngine"
+ /**
+ * Add or override spark.master if KUBERNETES_SERVICE_HOST and
+ * KUBERNETES_SERVICE_PORT are defined
+ */
+ protected def initialize(): Unit = {
+ try {
+ if (conf.get(KUBERNETES_MASTER_OVERRIDE)) {
Review Comment:
how about trying to auto configure it iff `spark.master` is absent?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on pull request #3852: [KYUUBI #3851][SPARK] Support auto set up `spark.master` when Kyuubi running inside Pod
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1328661548
"CI / Kyuubi Server On Kubernetes Integration Test" failed, and it's related
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter commented on pull request #3852: [KYUUBI #3851][SPARK] Support auto set up `spark.master` when Kyuubi running inside Pod
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1329003214
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/3852?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3852](https://codecov.io/gh/apache/incubator-kyuubi/pull/3852?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8bfa253) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/35669b375a78d855c5bd33577e9928be1606fb5c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (35669b3) will **increase** coverage by `0.03%`.
> The diff coverage is `66.66%`.
> :exclamation: Current head 8bfa253 differs from pull request most recent head d83e814. Consider uploading reports for the commit d83e814 to get more accurate results
```diff
@@ Coverage Diff @@
## master #3852 +/- ##
============================================
+ Coverage 52.94% 52.98% +0.03%
Complexity 13 13
============================================
Files 497 497
Lines 28023 28086 +63
Branches 3864 3868 +4
============================================
+ Hits 14838 14880 +42
- Misses 11784 11801 +17
- Partials 1401 1405 +4
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/3852?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ache/kyuubi/engine/spark/SparkProcessBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3852/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvc3BhcmsvU3BhcmtQcm9jZXNzQnVpbGRlci5zY2FsYQ==) | `84.15% <60.00%> (-2.51%)` | :arrow_down: |
| [...kyuubi/engine/KubernetesApplicationOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3852/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvS3ViZXJuZXRlc0FwcGxpY2F0aW9uT3BlcmF0aW9uLnNjYWxh) | `24.24% <100.00%> (+2.36%)` | :arrow_up: |
| [...rg/apache/kyuubi/engine/spark/SparkSQLEngine.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3852/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9TcGFya1NRTEVuZ2luZS5zY2FsYQ==) | `70.10% <0.00%> (-5.49%)` | :arrow_down: |
| [...rg/apache/kyuubi/ctl/cmd/log/LogBatchCommand.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3852/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWN0bC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jdGwvY21kL2xvZy9Mb2dCYXRjaENvbW1hbmQuc2NhbGE=) | `63.93% <0.00%> (-1.64%)` | :arrow_down: |
| [...ache/kyuubi/server/KyuubiRestFrontendService.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3852/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpUmVzdEZyb250ZW5kU2VydmljZS5zY2FsYQ==) | `82.95% <0.00%> (-0.96%)` | :arrow_down: |
| [.../apache/kyuubi/server/api/v1/BatchesResource.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3852/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvYXBpL3YxL0JhdGNoZXNSZXNvdXJjZS5zY2FsYQ==) | `69.00% <0.00%> (-0.20%)` | :arrow_down: |
| [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3852/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `97.48% <0.00%> (-0.07%)` | :arrow_down: |
| [.../org/apache/kyuubi/config/KyuubiReservedKeys.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3852/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpUmVzZXJ2ZWRLZXlzLnNjYWxh) | `0.00% <0.00%> (ø)` | |
| [...apache/kyuubi/server/api/v1/SessionsResource.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3852/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvYXBpL3YxL1Nlc3Npb25zUmVzb3VyY2Uuc2NhbGE=) | `72.26% <0.00%> (+0.20%)` | :arrow_up: |
| ... and [6 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/3852/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] xuefeimiaoao commented on a diff in pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
xuefeimiaoao commented on code in PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#discussion_r1033078989
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -53,8 +54,42 @@ class SparkProcessBuilder(
Paths.get(sparkHome, "bin", SPARK_SUBMIT_FILE).toFile.getCanonicalPath
}
+ completeMasterUrl()
+
override def mainClass: String = "org.apache.kyuubi.engine.spark.SparkSQLEngine"
+ /**
+ * Add `spark.master` if KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT
+ * are defined. So we can deploy spark on kubernetes without setting `spark.master`
+ * explicitly when kyuubi-servers are on kubernetes, which also helps in case that
+ * api-server is not exposed to us.
+ * Besides, set `spark.master` into kyuubiConf if which is set in spark-defaults.conf
+ * under SPARK_CONF_DIR.
+ */
+ protected def completeMasterUrl(): Unit = {
+ try {
+ (
+ clusterManager(),
+ sys.env.get(KUBERNETES_SERVICE_HOST),
+ sys.env.get(KUBERNETES_SERVICE_PORT)) match {
+ case (None, Some(kubernetesServiceHost), Some(kubernetesServicePort)) =>
+ // According to "https://kubernetes.io/docs/concepts/architecture/control-plane-
+ // node-communication/#node-to-control-plane", the API server is configured to listen
+ // for remote connections on a secure HTTPS port (typically 443), so we set https here.
+ val masterURL = s"k8s://https://${kubernetesServiceHost}:${kubernetesServicePort}"
+ conf.set(MASTER_KEY, masterURL)
+ case (Some(clusterManager), _, _) =>
+ // in case that ClusterManager was set in spark-defaults.conf, which should be set into
+ // KyuubiConf, the one which is used to build sparkSubmit.
+ conf.setIfMissing(MASTER_KEY, clusterManager)
Review Comment:
How about complete spark.master if which is set into spark-defaults.conf and not in kyuubiConf, in which case spark.master will be lost when build sparkSubmit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] xuefeimiaoao commented on pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
xuefeimiaoao commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1328576959
>
I suppose that the default value of `spark.kubernetes.driver.master` in spark is just ok.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] xuefeimiaoao commented on a diff in pull request #3852: [KYUUBI #3851] [Improvement] Support override of spark.master with ku…
Posted by GitBox <gi...@apache.org>.
xuefeimiaoao commented on code in PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#discussion_r1033076688
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -53,8 +54,34 @@ class SparkProcessBuilder(
Paths.get(sparkHome, "bin", SPARK_SUBMIT_FILE).toFile.getCanonicalPath
}
+ completeMasterUrl()
+
override def mainClass: String = "org.apache.kyuubi.engine.spark.SparkSQLEngine"
+ /**
+ * Add `spark.master` if KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT
+ * are defined. So we can deploy spark on kubernetes without setting `spark.master`
+ * explicitly when kyuubi-servers are on kubernetes, which also helps in case that
+ * api-server is not exposed to us.
+ */
+ protected def completeMasterUrl(): Unit = {
+ try {
+ val kubernetesServiceHost = sys.env.get(KUBERNETES_SERVICE_HOST)
+ val kubernetesServicePort = sys.env.get(KUBERNETES_SERVICE_PORT)
+ if (conf.getOption(MASTER_KEY).isEmpty
Review Comment:
Besides, i found that `spark.master` will be lost if which is set under SPARK_CONF_DIR, so i add some code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] xuefeimiaoao commented on pull request #3852: [KYUUBI #3851][SPARK] Support auto set up `spark.master` when Kyuubi running inside Pod
Posted by GitBox <gi...@apache.org>.
xuefeimiaoao commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1329230812
I am confused how `org.apache.kyuubi.kubernetes.test.deployment.KyuubiOnKubernetesWithLocalSparkTestsSuite#connectionConf` passed to kyuubi server when testing. @zwangsheng
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] xuefeimiaoao commented on pull request #3852: [KYUUBI #3851][SPARK] Support auto set up `spark.master` when Kyuubi running inside Pod
Posted by GitBox <gi...@apache.org>.
xuefeimiaoao commented on PR #3852:
URL: https://github.com/apache/incubator-kyuubi/pull/3852#issuecomment-1338625851
> Hi @xuefeimiaoao, sorry for late fix IT test.
>
> Kyuubi On Kubernetes IT Test will be fixed in this #3869 PR, and your can rebase this pr after it's meraged.
Cool, thx!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org