Copilot commented on code in PR #13948:
URL: https://github.com/apache/gravitino/pull/13948#discussion_r4312229437
##########
plugins/k8s-job-executor/src/main/java/org/apache/gravitino/job/k8s/spark/SparkApplicationUtils.java:
##########
@@ -224,6 +236,18 @@ private static String getExecutableUri(
return template.executable();
}
+ private static void checkEnvironmentName(SparkJobTemplate template, String
name) {
+ // Checked here, as Kubernetes would otherwise reject the name only when
the operator creates
+ // the driver pod.
+ if (!ENVIRONMENT_NAME_PATTERN.matcher(name).matches()) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Environment variable name %s of job template %s is invalid: it
must consist of"
+ + " letters, digits, '_', '-' or '.', and must not start
with a digit",
+ name, template.name()));
Review Comment:
The PR description says invalid environment variable names are rejected if
they don’t match `[-._a-zA-Z][-._a-zA-Z0-9]*`, but the implementation
additionally rejects `"."` and any name starting with `".."` (both of which
match that regex). If the extra restrictions are intentional for Kubernetes
compatibility across versions (as the comment suggests), please update the PR
description to reflect the stricter rule; otherwise, align the validation to
the stated regex-only requirement.
##########
plugins/k8s-job-executor/src/main/java/org/apache/gravitino/job/k8s/spark/SparkApplicationUtils.java:
##########
@@ -224,6 +236,18 @@ private static String getExecutableUri(
return template.executable();
}
+ private static void checkEnvironmentName(SparkJobTemplate template, String
name) {
+ // Checked here, as Kubernetes would otherwise reject the name only when
the operator creates
+ // the driver pod.
+ if (!ENVIRONMENT_NAME_PATTERN.matcher(name).matches()) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Environment variable name %s of job template %s is invalid: it
must consist of"
+ + " letters, digits, '_', '-' or '.', and must not start
with a digit",
+ name, template.name()));
+ }
+ }
+
/**
* Checks that a resource of the job, or each of them if it is a
comma-separated list as Spark
* accepts, is a URI that the cluster can reach rather than a path on the
Gravitino server.
Review Comment:
This is a user-facing error; the textual rules can drift from the actual
validation logic (regex + special cases), and it doesn’t explicitly mention the
non-empty requirement (even though `""` is rejected). Consider making the
message more directly match the code by including the exact allowed pattern and
explicitly calling out the additional disallowed cases (e.g., empty, `"."`,
prefix `".."`) so users can fix their templates without guessing.
##########
plugins/k8s-job-executor/src/main/java/org/apache/gravitino/job/k8s/spark/SparkApplicationUtils.java:
##########
@@ -106,14 +118,7 @@ private SparkApplicationUtils() {}
*/
public static GenericKubernetesResource buildSparkApplication(
JobContext context, SparkJobTemplate template, K8sJobExecutorConfigs
configs) {
- if (!template.environments().isEmpty()) {
- throw new IllegalArgumentException(
- String.format(
- "Job template %s sets environment variables %s, which the k8s
job executor doesn't"
- + " support yet. Pass them as Spark configurations instead,
for example"
- + " spark.kubernetes.driverEnv.<name> and
spark.executorEnv.<name>",
- template.name(), template.environments().keySet()));
- }
+ template.environments().keySet().forEach(name ->
checkEnvironmentName(template, name));
String executable = getExecutableUri(context, template, configs);
checkClusterReachable(executable);
Review Comment:
The template environments are iterated twice (once to validate keys, once to
populate `sparkConf`). Consider folding validation into the population loop
(validate `name` before inserting) so the mapping logic is in one place and
less likely to get out of sync over time.
##########
plugins/k8s-job-executor/src/main/java/org/apache/gravitino/job/k8s/spark/SparkApplicationUtils.java:
##########
@@ -122,6 +127,13 @@ public static GenericKubernetesResource
buildSparkApplication(
template.archives().forEach(SparkApplicationUtils::checkClusterReachable);
Map<String, String> sparkConf = new TreeMap<>(configs.sparkConf());
+ template
+ .environments()
+ .forEach(
+ (name, value) -> {
+ sparkConf.put(SPARK_DRIVER_ENV_PREFIX + name, value);
+ sparkConf.put(SPARK_EXECUTOR_ENV_PREFIX + name, value);
+ });
sparkConf.putAll(template.configs());
// The operator only supports cluster mode, so a job that asks for client
mode is rejected
// rather than silently run in cluster mode.
Review Comment:
The template environments are iterated twice (once to validate keys, once to
populate `sparkConf`). Consider folding validation into the population loop
(validate `name` before inserting) so the mapping logic is in one place and
less likely to get out of sync over time.
--
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]
