dongjoon-hyun commented on a change in pull request #32610: URL: https://github.com/apache/spark/pull/32610#discussion_r649653101
########## File path: docs/running-on-kubernetes.md ########## @@ -889,7 +889,10 @@ See the [configuration page](configuration.html) for information on Spark config <td><code>spark.kubernetes.executor.podNamePrefix</code></td> <td>(none)</td> <td> - Prefix to use in front of the executor pod names. + Prefix to use in front of the executor pod names. It must conform the rules defined by the Kubernetes + <a href="https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names";>DNS Label Names</a>. + Besides, the executor pod names will be generated in the form of <code>$podNamePrefix-exec-$id</code>, where the `id` is + a positive int value, so the length of the `podNamePrefix` need to be less than or equal to 47(= 63 - 10 - 6). Review comment: `need` -> `needs`? ########## File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtils.scala ########## @@ -29,13 +29,16 @@ import io.fabric8.kubernetes.api.model.{ConfigMap, ConfigMapBuilder, KeyToPath} import org.apache.spark.SparkConf import org.apache.spark.deploy.k8s.{Config, Constants, KubernetesUtils} +import org.apache.spark.deploy.k8s.Config.KUBERNETES_LABEL_MAX_LENGTH import org.apache.spark.deploy.k8s.Constants.ENV_SPARK_CONF_DIR import org.apache.spark.internal.Logging private[spark] object KubernetesClientUtils extends Logging { // Config map name can be 63 chars at max. - def configMapName(prefix: String): String = s"${prefix.take(54)}-conf-map" + def configMapName(prefix: String, suffix: String = "-conf-map"): String = { Review comment: Do we have any place using `suffix` parameter? ########## File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientUtils.scala ########## @@ -29,13 +29,16 @@ import io.fabric8.kubernetes.api.model.{ConfigMap, ConfigMapBuilder, KeyToPath} import org.apache.spark.SparkConf import org.apache.spark.deploy.k8s.{Config, Constants, KubernetesUtils} +import org.apache.spark.deploy.k8s.Config.KUBERNETES_LABEL_MAX_LENGTH import org.apache.spark.deploy.k8s.Constants.ENV_SPARK_CONF_DIR import org.apache.spark.internal.Logging private[spark] object KubernetesClientUtils extends Logging { // Config map name can be 63 chars at max. - def configMapName(prefix: String): String = s"${prefix.take(54)}-conf-map" + def configMapName(prefix: String, suffix: String = "-conf-map"): String = { Review comment: Do we have any place using `suffix` parameter? It looks like introducing inconsistency if we recommend to create a new config map name with different suffix. In other words, let's not change the function signature to have consistent Spark-generate config map names. -- 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
