jerryshao commented on code in PR #13982: URL: https://github.com/apache/gravitino/pull/13982#discussion_r4374473876
########## 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: Added the same caveat in 8fed16dd1f: spark.jars in spark_conf replaces a spark.jars default of the k8s job executor, so list every jar the job needs there. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_ ########## 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: Closed it in 8fed16dd1f: a new checkExecutorEnvironmentName helper now runs on the template's configs as well as on spark_conf, so spark.executorEnv.<invalid name> in a template's configs is rejected at submit time too. The test covers both. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_ -- 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]
