jerryshao commented on code in PR #13700:
URL: https://github.com/apache/gravitino/pull/13700#discussion_r4281018161
##########
plugins/k8s-job-executor/src/main/java/org/apache/gravitino/job/k8s/spark/SparkApplicationStatusUtils.java:
##########
@@ -188,7 +189,15 @@ public static JobExecutionInfo executionInfoOf(
status = outcome == null ? JobHandle.Status.FAILED : statusOf(outcome);
finishedAt = stoppedAt;
} else if (current == null) {
- if (createdAt != null &&
createdAt.plusMillis(noStatusTimeoutMs).isBefore(now)) {
+ if (hasStatus(app)) {
+ // The operator wrote a status this version can't read, for example a
newer operator. The
+ // application is known to the operator, so the job stays unfinished
rather than failing.
+ status = JobHandle.Status.QUEUED;
Review Comment:
[Nit] Keeping such a job unfinished is the right call, but nothing ends it
afterwards, and the status can go backwards.
Two consequences worth a line of documentation:
- A job that had readable states and then gets a status this version can't
read (an operator upgraded while it runs) is reported QUEUED, and `JobManager`
persists that: `toUpdatedStatusJobEntity` only refuses to regress out of a
terminal state or out of CANCELLING
(`core/src/main/java/org/apache/gravitino/job/JobManager.java:1219-1228`), so
STARTED -> QUEUED is written. The recorded `startedAt` survives
(`resolveStartedAt`, `JobManager.java:1263-1282`).
- Such a job never settles by itself. `K8sJobExecutor` doesn't override
`isJobStateNodeLocal`, so `cleanUpStagingDirs` never expires an active job of
this executor (`JobManager.java:776-780`: "Jobs of other job executors can be
tracked by any server, so they never expire"). It only ends when the operator
deletes the SparkApplication `spark.ttlAfterStopInMs` after it stops, 7 days by
default (`K8sJobExecutorConfigs.java:138-141`); the GET then finds nothing and
the job is FAILED, even if it succeeded. Until then this warning is logged on
every status poll of every such job.
Nothing to change in the mapping - the application is known to the operator
and the old behaviour was worse. Worth stating on `noStatusTimeoutInMs`, or in
the class Javadoc, that a status this version can't read leaves the job
unfinished until the application's TTL removes it, so an operator reads it as
"upgrade mismatch, act" rather than waiting it out.
Verified by: reading this branch and `hasStatus`
(`SparkApplicationStatusUtils.java:261-264`), `K8sJobExecutor.java:424-428`,
`JobManager.java:776-780`, `:1219-1228`, `:1263-1282` and
`K8sJobExecutorConfigs.java:138-141` on d7746f31d5. Not reproduced: no build or
test run was possible here (JDK 21 only, the toolchain needs 17).
##########
plugins/k8s-job-executor/src/main/java/org/apache/gravitino/job/k8s/spark/SparkApplicationUtils.java:
##########
@@ -117,6 +118,16 @@ public static GenericKubernetesResource
buildSparkApplication(
Map<String, String> sparkConf = new TreeMap<>(configs.sparkConf());
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.
+ String deployMode =
StringUtils.trimToNull(sparkConf.get(SPARK_DEPLOY_MODE));
+ if (deployMode != null &&
!CLUSTER_DEPLOY_MODE.equalsIgnoreCase(deployMode)) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Job template %s sets %s to %s, but the k8s job executor only
runs Spark jobs in %s"
+ + " mode",
+ template.name(), SPARK_DEPLOY_MODE, deployMode,
CLUSTER_DEPLOY_MODE));
+ }
Review Comment:
[Nit] The message blames the job template for a value that may come from the
executor configuration.
`sparkConf` is the executor's `spark.conf.*` merged with the template's
configs (`:119-120`), so a deployment that sets
`gravitino.jobExecutor.k8s.spark.conf.spark.submit.deployMode=client` fails
every job with "Job template <name> sets spark.submit.deployMode to client",
pointing at templates that set nothing. This PR's own test takes exactly that
path: `TestSparkApplicationUtils.java:56-67` builds a template with no configs
and the executor configuration set to `client`.
Naming the key without attributing it to the template, for example "the job
template or the k8s job executor configuration sets ...", would send the
operator to the right place.
Verified by: reading `SparkApplicationUtils.java:119-130` and
`TestSparkApplicationUtils.java:42-67` on d7746f31d5.
##########
plugins/k8s-job-executor/src/main/java/org/apache/gravitino/job/k8s/spark/SparkApplicationUtils.java:
##########
@@ -117,6 +118,16 @@ public static GenericKubernetesResource
buildSparkApplication(
Map<String, String> sparkConf = new TreeMap<>(configs.sparkConf());
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.
+ String deployMode =
StringUtils.trimToNull(sparkConf.get(SPARK_DEPLOY_MODE));
+ if (deployMode != null &&
!CLUSTER_DEPLOY_MODE.equalsIgnoreCase(deployMode)) {
Review Comment:
[Question] Should `spark.master` be treated the same way?
It is still dropped silently two lines below (`:131-133`), and
`testBuildSparkApplication` passes `spark.master=local[*]` and asserts it is
removed (`TestSparkApplicationUtils.java:84`, `:310`). A template written for
local execution - local master, local paths, a tiny driver - then runs
distributed on the cluster instead of being rejected, which is the same class
of surprise as client mode, and arguably a more expensive one.
If dropping it is deliberate, because the operator always sets the master
and a template that carries `spark.master` from a copy-paste shouldn't fail,
then this is fine as it is and only the comment on `:131` need say so.
Otherwise rejecting a master that isn't the operator's would be consistent with
the check being added here.
Verified by: reading `SparkApplicationUtils.java:119-133` and
`TestSparkApplicationUtils.java:72-90`, `:280-311` on d7746f31d5.
--
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]
