dongjoon-hyun commented on code in PR #58696:
URL: https://github.com/apache/spark/pull/58696#discussion_r3990423695
##########
core/src/main/scala/org/apache/spark/deploy/Client.scala:
##########
@@ -101,9 +102,10 @@ private class ClientEndpoint(
val sparkJavaOpts = Utils.sparkJavaOpts(conf)
val javaOpts = sparkJavaOpts ++ extraJavaOpts
+ val driverEnv = Client.driverEnvironment(conf, sys.env)
Review Comment:
One thing worth being aware of: this is not only about what the driver JVM
inherits at runtime. The worker consults `command.environment` directly in a
few places, and those lookups take precedence over the worker's own env:
- `WorkerCommandBuilder` copies `command.environment` into `childEnv`, and
`AbstractCommandBuilder.buildJavaCommand` checks `childEnv.get("JAVA_HOME")`
before `System.getenv("JAVA_HOME")`; `buildClassPath` reads `HADOOP_CONF_DIR` /
`YARN_CONF_DIR` via `getenv`, which also prefers `childEnv`.
- `CommandUtils.buildLocalCommand` reads `LD_LIBRARY_PATH` /
`DYLD_LIBRARY_PATH` from `command.environment` when
`spark.driver.extraLibraryPath` is set.
Since `useRest` defaults to `false` in `SparkSubmitArguments`, this legacy
path is the default standalone cluster gateway, so a submitter that relied on
e.g. `HADOOP_CONF_DIR` or `LD_LIBRARY_PATH` reaching the driver will see a
change on upgrade. I think matching the REST behavior is the right call, but it
is more than a cosmetic change, which is another reason to make the doc precise
(see the comment on the config doc) and to mention these concretely in the
migration guide entry.
##########
core/src/main/scala/org/apache/spark/deploy/Client.scala:
##########
@@ -275,6 +277,21 @@ object Client {
// scalastyle:on println
new ClientApp().start(args, new SparkConf())
}
+
+ /**
+ * Environment variables to forward to the driver. Only Spark-related
variables are forwarded,
+ * matching the REST submission client, unless
`spark.standalone.submit.filterEnvironment` is
+ * disabled, in which case the full environment of the submitting process is
forwarded.
+ */
+ private[deploy] def driverEnvironment(
Review Comment:
The new config is only honored here. `RestSubmissionClientApp.start` still
calls `RestSubmissionClient.filterSystemEnvironment(sys.env)` unconditionally,
so with `spark.master.rest.enabled=true` setting
`spark.standalone.submit.filterEnvironment=false` has no effect. And because
`SparkSubmit.submit` falls back from REST to this `Client` on
`SubmitRestConnectionException`, the same submission can end up with a
different driver environment depending on which gateway answered.
Either apply this helper in the REST client too (it could live in a neutral
place instead of `Client` depending on `deploy.rest`), or state in the doc that
the config only applies to the legacy RPC gateway.
##########
core/src/test/scala/org/apache/spark/deploy/ClientSuite.scala:
##########
@@ -48,4 +49,19 @@ class ClientSuite extends SparkFunSuite with Matchers {
// Invalid syntax.
ClientArguments.isValidJarUrl("hdfs:") should be (false)
}
+
+ test("SPARK-59404: forward only Spark-related environment variables to the
driver") {
Review Comment:
Both tests exercise `Client.driverEnvironment` directly, and the filtering
rule itself is already covered by `StandaloneRestSubmitSuite`. Nothing checks
that `ClientEndpoint` actually puts the filtered map into
`DriverDescription.command.environment`, so restoring `sys.env` at the `new
Command(...)` call would keep these green. Would it be feasible to assert on
the `RequestSubmitDriver` message (or the built `Command`) instead?
##########
docs/spark-standalone.md:
##########
@@ -575,6 +575,16 @@ Spark applications supports the following configuration
properties specific to s
</td>
<td>3.1.0</td>
</tr>
+ <tr>
+ <td><code>spark.standalone.submit.filterEnvironment</code></td>
Review Comment:
The sibling `spark.standalone.submit.waitAppCompletion` is also listed in
`docs/configuration.md`. Shall we add this one there as well for consistency?
##########
core/src/main/scala/org/apache/spark/internal/config/package.scala:
##########
@@ -2787,6 +2787,16 @@ package object config {
.booleanConf
.createWithDefault(false)
+ private[spark] val STANDALONE_SUBMIT_FILTER_ENVIRONMENT =
+ ConfigBuilder("spark.standalone.submit.filterEnvironment")
+ .doc("In standalone cluster mode, controls whether the client forwards
only " +
+ "Spark-related environment variables to the driver, matching the REST
submission " +
Review Comment:
"Spark-related environment variables" is hard for users to act on. Could we
state the actual rule, i.e. variables whose name starts with `SPARK_`,
excluding `SPARK_HOME`, `SPARK_CONF_DIR` and `SPARK_ENV_LOADED`? Otherwise it
is not obvious that `SPARK_HOME` is dropped while `HADOOP_CONF_DIR` is too.
Same for the `docs/spark-standalone.md` row and the scaladoc on
`Client.driverEnvironment`.
--
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]
