Yikun commented on a change in pull request #35215:
URL: https://github.com/apache/spark/pull/35215#discussion_r790227632
##########
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
> driver confileMaps sets namespace in the same ways
Do you mean [the same
way](https://github.com/apache/spark/blob/c4a9772f741836cdb399e35a45b3b5df48d9eea6/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtils.scala#L92)
is what was introduced in https://github.com/apache/spark/pull/34983 ?
Actually this is the reason of why we need this PR (regression for driver side)
and where the `confFileMap` mixup problem happened.
> aiming to keep confMaps up with sparkConf.
Aggree, this is right, and this PR also can keep this regression, you can
see @chia7712 already added the test for driver and executor and also avoid
`confFileMap` mixup problem.
> Second, spark.k8s.executor.configmap.namespace is not necessary.
I said "**if we need**", I updated it in my last comments to avoid
misleading. And also it decribed how we set namespace according conf (even if
the potential separate configure case in future) rather a indirect step
confFileMap.
##########
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala
##########
@@ -76,13 +76,12 @@ private[spark] class KubernetesClusterSchedulerBackend(
private def setUpExecutorConfigMap(driverPod: Option[Pod]): Unit = {
val configMapName = KubernetesClientUtils.configMapNameExecutor
- val resolvedExecutorProperties =
- Map(KUBERNETES_NAMESPACE.key -> conf.get(KUBERNETES_NAMESPACE))
val confFilesMap = KubernetesClientUtils
Review comment:
Of course, this is just my prefer, so I give my +1 on this PR. If the
confiFilemap way can **solve the key mixup problem** and **also have regresson
on driver side**, I'd also okay with it.
Anyway, both two way can also solve the problem of driver side, I think we
need to get problem fixed first because this is a very basic 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
