jerryshao commented on code in PR #13982: URL: https://github.com/apache/gravitino/pull/13982#discussion_r4333840277
########## docs/table-maintenance-service/optimizer-configuration.md: ########## @@ -110,8 +110,11 @@ that lists the missing keys. See and the defaults of each template. Built-in Iceberg templates also need an Iceberg Spark runtime on the Spark classpath. They do not -ship that JAR or fill template `jars`, so include it yourself — for example -`"spark.jars":"/path/to/iceberg-spark-runtime-....jar"` inside `spark_conf`. Match the artifact to +ship that JAR or fill template `jars`, so include it yourself — for example by installing it into +`SPARK_HOME/jars`, or, with the k8s job executor, by +`"spark.jars":"https://.../iceberg-spark-runtime-....jar"` inside `spark_conf`. With the local job Review Comment: [Nit] This paragraph is the one place left that offers `spark.jars` in `spark_conf` without the caveat the rest of the docs gained in 2cd6eb9. `optimizer-cli-reference.md:186-187` and `:261-263` and `optimizer-troubleshooting.md:75-76` now all say that `spark.jars` in `spark_conf` replaces a `spark.jars` default of the k8s job executor rather than adding to it. This passage, in a file this PR also changes, still reads as if it only adds the Iceberg runtime, so a deployment whose executor default carries something else (the JDBC driver case in `optimizer-cli-reference.md:183-187`) loses it by following this page. Suggest the same half-sentence here, e.g. after `inside spark_conf` on line 115: "which replaces a `spark.jars` default of the k8s job executor, so list every jar the job needs there". Verified by: grepped `spark.jars` across `docs/table-maintenance-service/*.md` on 2cd6eb9 and read all five hits; read the three doc hunks of this commit. ########## plugins/k8s-job-executor/src/main/java/org/apache/gravitino/job/k8s/spark/SparkApplicationUtils.java: ########## @@ -216,6 +260,115 @@ public static GenericKubernetesResource buildSparkApplication( return app; } + /** + * Returns the Spark configurations that a job sets itself, which override the default Spark + * configurations of the job executor. + * + * <p>They are the configurations of the job template, and for a built-in job also those of its + * {@code spark_conf} parameter, which override them. The driver of a built-in job applies its + * {@code spark_conf} only once it starts, too late for the configurations that spark-submit + * reads, such as {@code spark.jars}, so they are set in the SparkApplication as well. {@code + * spark.master} and {@code spark.submit.deployMode} there are ignored, as the driver also ignores + * them, and so are the {@code spark.kubernetes.*} configurations, which only the job executor + * configurations and job templates may set: whoever can run a built-in job sets its {@code + * spark_conf}. A blank value leaves a configuration unset, as it is what an optional template + * parameter becomes, so it doesn't override the default of the job executor. + */ + private static Map<String, String> getJobSparkConf( + JobContext context, SparkJobTemplate template) { + Map<String, String> jobSparkConf = new TreeMap<>(); + template + .configs() + .forEach( + (key, value) -> { + if (StringUtils.isNotBlank(value)) { + jobSparkConf.put(key, value); + } + }); + if (!template.name().startsWith(JobTemplateProvider.BUILTIN_NAME_PREFIX)) { + return jobSparkConf; + } + + getBuiltinSparkConf(template) + .forEach( + (key, value) -> { + if (StringUtils.isBlank(value)) { + return; + } + if (SUBMIT_DECIDED_CONFIGS.contains(key)) { + LOG.warn( + "Ignored {}={} in spark_conf of job {} of built-in job template {}, as the" + + " Spark operator decides it", + key, + value, + context.jobId(), + template.name()); + } else if (key.startsWith(SPARK_KUBERNETES_PREFIX)) { + // The value isn't logged, as it may name a Secret. + LOG.warn( + "Ignored {} in spark_conf of job {} of built-in job template {}, as only the" + + " k8s job executor configurations and job templates may set {}*", + key, + context.jobId(), + template.name(), + SPARK_KUBERNETES_PREFIX); + } else { + // Checked like the environment variables of the template, as an invalid name + // would only fail the executor pods. + if (key.startsWith(SPARK_EXECUTOR_ENV_PREFIX)) { + checkEnvironmentName(template, key.substring(SPARK_EXECUTOR_ENV_PREFIX.length())); Review Comment: [Nit] The new check covers `spark_conf`, but the same keys arriving through the template's `configs` still skip it. `getJobSparkConf` copies `template.configs()` straight into `jobSparkConf` at :280-287 with only a blankness test, and that map lands in `spec.sparkConf` through the `putAll` at :167. So `spark.executorEnv.BAD NAME` set as a template config reaches the SparkApplication unvalidated and fails the executor pods later, which is exactly the failure mode this check and the `template.environments()` check at :163 exist to turn into a submit-time rejection. It is a smaller hole than the `spark_conf` one - a template is registered by an operator, not by whoever runs the job - so it may be fine to leave. If so, consider saying in the Javadoc of `getJobSparkConf` that only `spark_conf` env names are checked; otherwise running the same `startsWith(SPARK_EXECUTOR_ENV_PREFIX)` test in the `template.configs()` loop would close it in three lines. Verified by: grepped `checkEnvironmentName` in SparkApplicationUtils.java on 2cd6eb9 - the only call sites are :163 (template environments) and :319 (this one); read `getJobSparkConf` :277-325 and the merge at :158-168. -- 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]
