Support executor java options #445
Conversation
mccheah
changed the base branch from
branch-2.2-kubernetes
to
support-java-options-with-spaces
| val executorSysProps = spark.parallelize(Seq(0)).flatMap { _ => | ||
| Maps.fromProperties(System.getProperties).asScala | ||
| }.collectAsMap() | ||
| println(s"The executor's JVM options did not match. Expected" + |
There was a problem hiding this comment.
I'm wondering why would you use println here instead of logger output?
There was a problem hiding this comment.
because this is integration test code, and we only get the container output (its stdout) so using a logger only adds more variability to the output.
See runSparkApplicationAndVerifyCompletion's expectedLogOnCompletion parameter
| println(s"All expected JVM options were present on the driver and executors.") | ||
| val spark = SparkSession.builder().getOrCreate().sparkContext | ||
| try { | ||
| val nonMatchingExecutorOptions = spark.parallelize(Seq(0)).flatMap { _ => |
There was a problem hiding this comment.
note to other reviewers: the lambda parameter to this flatMap will run on executors, not driver
| val executorSysProps = spark.parallelize(Seq(0)).flatMap { _ => | ||
| Maps.fromProperties(System.getProperties).asScala | ||
| }.collectAsMap() | ||
| println(s"The executor's JVM options did not match. Expected" + |
There was a problem hiding this comment.
because this is integration test code, and we only get the container output (its stdout) so using a logger only adds more variability to the output.
See runSparkApplicationAndVerifyCompletion's expectedLogOnCompletion parameter
| println(s"Key: ${prop._1}, Value: ${prop._2}") | ||
| } | ||
| } else { | ||
| println("All expected JVM options were present on the driver and executors.") |
There was a problem hiding this comment.
you use System.exit(1) to fail out of the driver section but have an if here. I think I'd prefer using System.exit in both places
|
Merge #444 first |
ash211
changed the base branch from
support-java-options-with-spaces
to
branch-2.2-kubernetes
|
@mccheah haven't figured out why PRs A <- B <- C can all be clean, and then when you merge A <-B and retarget A <- C there are conflicts. Bottom line is, merge conflicts |
|
I've been using a different workflow to retarget these chained PRs:
Doing it this way doesn't introduce any conflicts. |
mccheah
force-pushed
the
support-executor-java-options
branch
from
ad04a8d to
9d8e981
Compare
| If this is the first time you compile the Kubernetes core implementation module, run the following command to install the dependencies and compile: | ||
|
|
||
| build/mvn install -Pkubernetes -pl resource-managers/kubernetes/core -am -DskipTests | ||
|
|
mccheah
force-pushed
the
support-executor-java-options
branch
from
ae0f1f7 to
e03492b
Compare
ash211
left a comment
ash211
left a comment
There was a problem hiding this comment.
Looks right now, ready to merge when tests pass
Closes #235 and requires #444.