jerryshao commented on code in PR #13948:
URL: https://github.com/apache/gravitino/pull/13948#discussion_r4312285681
##########
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 rule is intentional: Kubernetes without the relaxed environment variable
validation also rejects `.` and names starting with `..`. The PR description
was updated to state the full rule (regex, not `.`, not starting with `..`).
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
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:
Done in 364329f38: the message now gives the exact pattern from the code
(which also implies a non-empty name) plus the two extra cases, e.g. "it must
match [-._a-zA-Z][-._a-zA-Z0-9]*, must not be '.' and must not start with '..'".
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
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:
Done in 364329f38: each name is now checked in the loop that maps it to the
Spark configurations, so the environments are iterated once.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
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:
Done in 364329f38 (same as the other thread): the name check moved into this
loop, right before the two Spark configurations are set.
_🤖 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]
