Move executor pod construction to a separate class. #452
Conversation
| }.getOrElse(containerWithExecutorLimitCores) | ||
| val withMaybeShuffleConfigPod = shuffleServiceConfig.map { config => | ||
| config.shuffleDirs.foldLeft(executorPod) { (builder, dir) => | ||
| new PodBuilder(builder) |
There was a problem hiding this comment.
Ah we lost the indentation here, I'll fix that.
mccheah
force-pushed
the
support-executor-java-options
branch
from
ae0f1f7 to
e03492b
Compare
| ConfigurationUtils.parsePrefixedKeyValuePairs( | ||
| sparkConf, | ||
| KUBERNETES_NODE_SELECTOR_PREFIX, | ||
| "node-selector") |
There was a problem hiding this comment.
"node selector" for consistency?
There was a problem hiding this comment.
That would be a break in the configuration I think. Aside from that, SparkConf keys have never had spaces in them.
There was a problem hiding this comment.
I think that last string is only used for log output -- node selector seems like it would be fine
| "-DsimpleDriverConf=simpleDriverConfValue" + | ||
| " -Ddriverconfwithspaces='driver conf with spaces value'") | ||
| sparkConf.set("spark.files", driverJvmOptionsFile.getAbsolutePath) | ||
| sparkConf.set(SparkLauncher.EXECUTOR_EXTRA_JAVA_OPTIONS, |
There was a problem hiding this comment.
This is a separate change from the refactor correct?
|
I believe the diff is corrupted and I have to fix it in git. |
|
Actually I just need to rebase against branch-2.2-kubernetes and make the diff that way. |
This is the first of several measures to make KubernetesClusterSchedulerBackend feasible to test.
mccheah
changed the base branch from
support-executor-java-options
to
branch-2.2-kubernetes
There was a problem hiding this comment.
Believe this change is unintentional
mccheah
force-pushed
the
separate-executor-pod-construction
branch
from
dbb113d to
dc6b186
Compare
|
@foxish rebase complete. |
There was a problem hiding this comment.
please don't revert this change -- it went in in a PR and somehow your PRs keep coming close to reverting it.. ?
| import org.apache.spark.deploy.kubernetes.submit.{InitContainerUtil, MountSmallFilesBootstrap} | ||
| import org.apache.spark.util.Utils | ||
|
|
||
| // Strictly an extension of KubernetesClusterSchedulerBakcne that is factored out for testing. |
There was a problem hiding this comment.
by extension, you mean the method in this trait is the same as a method in KubernetesClusterSchedulerBackend ? That implies to me it should do multiple inheritance.
I'm not sure the "strictly an extension" language makes sense -- maybe instead say that it's only used in the scheduler backend?
There was a problem hiding this comment.
Extension meaning a plugin, or functionality that is pretty much only used in the scheduler backend.
| ConfigurationUtils.parsePrefixedKeyValuePairs( | ||
| sparkConf, | ||
| KUBERNETES_NODE_SELECTOR_PREFIX, | ||
| "node-selector") |
There was a problem hiding this comment.
I think that last string is only used for log output -- node selector seems like it would be fine
|
|
||
| import ExecutorPodFactoryImpl._ | ||
|
|
||
| private val EXECUTOR_ID_COUNTER = new AtomicLong(0L) |
There was a problem hiding this comment.
where is this used? I think it should be deleted because KubernetesClusterSchedulerBackend.scala already has one
| import org.apache.spark.deploy.kubernetes.constants.ANNOTATION_EXECUTOR_NODE_AFFINITY | ||
| import org.apache.spark.internal.Logging | ||
|
|
||
| // Strictly an extension of ExecutorPodFactory but extracted out for testing. |
There was a problem hiding this comment.
not sure this comment adds much -- it's good practice to have smaller more modular pieces anyway for understanding, regardless of testing purposes
| }.getOrElse(containerWithExecutorLimitCores) | ||
| val withMaybeShuffleConfigPod = shuffleServiceConfig.map { config => | ||
| config.shuffleDirs.foldLeft(executorPod) { (builder, dir) => | ||
| new PodBuilder(builder) |
|
@mccheah conflicts on the GiB -> MiB conversion fix since you moved that elsewhere |
…rate-executor-pod-construction
Move MiB change to ExecutorPodFactory.
|
rerun integration tests please |
|
Rerun unit tests please |
|
test coverage is unchanged (this is an internal refactor) and enables more granular testing in followup PRs |
…k8s#452) * Move executor pod construction to a separate class. This is the first of several measures to make KubernetesClusterSchedulerBackend feasible to test. * Revert change to README * Address comments. * Resolve merge conflicts. Move MiB change to ExecutorPodFactory.
…k8s#452) * Move executor pod construction to a separate class. This is the first of several measures to make KubernetesClusterSchedulerBackend feasible to test. * Revert change to README * Address comments. * Resolve merge conflicts. Move MiB change to ExecutorPodFactory.
This is the first of several measures to make KubernetesClusterSchedulerBackend feasible to test. Requires #445 but only for convenience and not semantically speaking.
The idea is to start breaking down the functionality of KubernetesClusterSchedulerBackend into multiple individually unit-test friendly units. The logic that builds the executor pod structure was by far the single longest method that could be isolated with relative ease.