yuqi1129 commented on code in PR #13132:
URL: https://github.com/apache/gravitino/pull/13132#discussion_r4002918424
##########
core/src/main/java/org/apache/gravitino/job/local/SparkProcessBuilder.java:
##########
@@ -51,18 +52,38 @@ public class SparkProcessBuilder extends
LocalProcessBuilder {
protected SparkProcessBuilder(SparkJobTemplate sparkJobTemplate, Map<String,
String> configs) {
super(sparkJobTemplate, configs);
- String sparkHome =
-
Optional.ofNullable(configs.get(SPARK_HOME)).orElse(System.getenv(ENV_SPARK_HOME));
+ this.sparkSubmit = resolveSparkSubmit(configs);
+ }
+
+ /**
+ * Resolves the spark-submit executable from the local job executor
configurations, falling back
+ * to the {@code SPARK_HOME} environment variable.
+ *
+ * @param configs The local job executor configurations.
+ * @return The absolute path of the spark-submit executable.
+ * @throws IllegalArgumentException If neither the Spark home configuration
nor the {@code
+ * SPARK_HOME} environment variable is set, or spark-submit is not found
or not executable.
+ */
+ static String resolveSparkSubmit(Map<String, String> configs) {
+ return resolveSparkSubmit(configs, System.getenv(ENV_SPARK_HOME));
+ }
+
+ @VisibleForTesting
+ static String resolveSparkSubmit(Map<String, String> configs, @Nullable
String envSparkHome) {
+ String sparkHome =
Optional.ofNullable(configs.get(SPARK_HOME)).orElse(envSparkHome);
Preconditions.checkArgument(
StringUtils.isNotBlank(sparkHome),
"gravitino.jobExecutor.local.sparkHome or SPARK_HOME environment
variable must"
+ " be set for Spark jobs");
- this.sparkSubmit = sparkHome + "/bin/spark-submit";
+ String sparkSubmit = sparkHome + "/bin/spark-submit";
Review Comment:
`resolveSparkSubmit` validates a relative `sparkHome` against the server
current working directory, but `SparkProcessBuilder.start()` sets the child
process working directory to the job staging directory. As a result, a value
such as `sparkHome=relative-spark` can pass `isFile()`/`canExecute()` here and
then fail asynchronously with `No such file or directory` when `ProcessBuilder`
starts it, which is the behavior this PR aims to eliminate. This also
contradicts the method return contract stating that the path is absolute. Could
we normalize with `new File(sparkHome, "bin/spark-submit").getAbsoluteFile()`,
return `getAbsolutePath()`, and cover the relative-path case in the test?
--
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]
