Copilot commented on code in PR #13982:
URL: https://github.com/apache/gravitino/pull/13982#discussion_r4322457662
##########
maintenance/jobs/src/main/java/org/apache/gravitino/maintenance/jobs/iceberg/IcebergJobUtils.java:
##########
@@ -34,13 +41,30 @@
*/
public final class IcebergJobUtils {
+ private static final Logger LOG =
LoggerFactory.getLogger(IcebergJobUtils.class);
+
private static final String ICEBERG_SPARK_CATALOG =
"org.apache.iceberg.spark.SparkCatalog";
/**
* Matches a job template placeholder that no job configuration value
replaced, e.g. {@code
* {{use_caching}}}.
*/
private static final Pattern UNRESOLVED_PLACEHOLDER_PATTERN =
Pattern.compile("^\\{\\{[^{}]*}}$");
+ /**
+ * The Spark configurations that spark-submit decides. Set in the custom
Spark configurations of a
+ * job, which the driver applies, they would override the submitted value,
e.g. run a job in local
+ * mode inside the driver pod of a cluster.
+ */
+ private static final Set<String> SUBMIT_DECIDED_CONFIGS =
+ ImmutableSet.of("spark.master", "spark.submit.deployMode");
+
+ /**
+ * The prefix of the Kubernetes configurations, which a job run may not set
in its custom Spark
+ * configurations: the driver creates the executor pods from them, with
their image, service
+ * account, Secrets, and volumes. Only the job executor configurations and
job templates set them.
+ */
+ private static final String SPARK_KUBERNETES_PREFIX = "spark.kubernetes.";
Review Comment:
The same “submit-decided configs” set and `spark.kubernetes.*` filtering
semantics now exist in both `IcebergJobUtils` (driver-side filtering) and
`SparkApplicationUtils` (submit-time merging). To avoid divergence over time
(e.g., one side updated with new keys/prefix rules while the other isn’t),
consider centralizing these constants/rules in a shared utility (or at minimum
adding a short comment pointing to the other implementation and stating they
must remain aligned).
##########
maintenance/optimizer/src/test/java/org/apache/gravitino/maintenance/optimizer/TestOptimizerCmd.java:
##########
@@ -497,7 +497,8 @@ void testSubmitUpdateStatsJobDryRunUsesConfigDefaults()
throws Exception {
Assertions.assertTrue(output[0].contains("jobTemplate=builtin-iceberg-update-stats"));
Assertions.assertTrue(output[0].contains("update_mode=stats"));
Assertions.assertTrue(output[0].contains("table_identifier=ab.t1"));
- Assertions.assertTrue(output[0].contains("spark_master=local[*]"));
+ // Without a configured master, the job executor decides it.
+ Assertions.assertTrue(output[0].contains("spark_master=, "));
Review Comment:
This assertion is tightly coupled to the exact formatting (including a
trailing comma and space). If the output format changes slightly (e.g., spacing
or delimiter ordering), the test will fail despite correct behavior. Consider
asserting more robustly (e.g., that `spark_master=` is present and the value is
empty, without depending on adjacent punctuation/whitespace).
##########
plugins/k8s-job-executor/src/main/java/org/apache/gravitino/job/k8s/spark/SparkApplicationUtils.java:
##########
@@ -147,14 +174,25 @@ public static GenericKubernetesResource
buildSparkApplication(
+ " the k8s job executor only runs Spark jobs in %s mode",
SPARK_DEPLOY_MODE, deployMode, template.name(),
CLUSTER_DEPLOY_MODE));
}
+ // Likewise, a job that asks for another master, such as local[*], is
rejected rather than
+ // silently run on Kubernetes.
+ String master = StringUtils.trimToNull(sparkConf.get(SPARK_MASTER));
+ if (master != null && !master.startsWith(K8S_MASTER_PREFIX)) {
+ throw new IllegalArgumentException(
+ String.format(
+ "%s is set to %s by job template %s or by the k8s job executor
configurations, but"
+ + " the k8s job executor only runs Spark jobs on Kubernetes.
Leave %s unset,"
+ + " for a built-in job by leaving the spark_master parameter
empty",
+ SPARK_MASTER, master, template.name(), SPARK_MASTER));
+ }
Review Comment:
The rejection message for a non-k8s `spark.master` mentions “for a built-in
job … leave the spark_master parameter empty”, but this branch also triggers
for non-built-in templates and for executor defaults; in those cases
“spark_master parameter” is misleading. Suggest adjusting the message to give a
general remediation (“remove/leave spark.master unset in template/executor
conf”) and optionally add a second sentence specific to built-in templates
(“for built-in templates, keep spark_master empty”) only when `template.name()`
indicates a built-in.
--
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]
