yuqi1129 commented on PR #12715:
URL: https://github.com/apache/gravitino/pull/12715#issuecomment-5478420313
Reviewed the full diff. The storage/mapper/PO plumbing looks complete and
consistent: all four `JobEntity.builder()` sites carry the new field, all six
job SELECTs plus both inserts and the update were changed, column counts match,
and the nullable TEXT column follows existing precedent.
Three things I think are worth addressing before merge:
**1. `JobInfo.fromJobEntity` can now throw, and it is called inside the
dispatcher's try blocks** —
`core/src/main/java/org/apache/gravitino/listener/api/info/JobInfo.java:90`
`toRuntimeJobTemplate` makes a previously total function partial:
`fromRuntimeJobTemplateJson` throws `RuntimeException` on malformed JSON,
Jackson throws `InvalidTypeIdException` on an unknown `jobType`, and
`DTOConverters.fromDTO` throws `IllegalArgumentException` on an unsupported
type (or builds a template that trips its own `Preconditions` when a required
field is missing).
`JobEventDispatcher` calls `JobInfo.fromJobEntity(job)` *inside* the try
block of `getJob` (line 212), `runJob` (236) and `cancelJob` (255), so one such
row fails the whole operation with a 500 and dispatches a failure event.
`cancelJob` is the worst case: `jobExecutor.cancelJob()` has already succeeded
and the entity has already been updated to `CANCELLING` by the time the event
is built, so the caller gets an error for an operation that did happen.
Concrete scenario: a metalake is written by a newer server that supports a
third job type and then rolled back — every `getJob`/`cancelJob` on those rows
500s. Suggest catching and logging inside `toRuntimeJobTemplate` and returning
`null`, matching the defensive stance this PR already takes in `toJobDTOs`.
**2. The tolerance for a bad stored template was only added to the list
path** —
`server/src/main/java/org/apache/gravitino/server/web/rest/JobOperations.java:514`
`toDTO(JobEntity)` is still called unguarded at lines 378, 418 and 444 (get
/ run / cancel job). The same corrupt or forward-incompatible
`runtime_job_template` value that `listJobs` now survives makes those three
endpoints return 500 permanently, i.e. the job becomes both unreadable and
uncancellable through the API. Either guard inside `toDTO` itself, or apply the
same fallback at the three single-job call sites.
**3. Resolved `jobConf` values are now persisted and returned** —
`core/src/main/java/org/apache/gravitino/job/JobManager.java:454`
`createRuntimeJobTemplate` substitutes caller-supplied `jobConf` values into
`arguments`, `environments`, `customFields` and (for Spark) `configs`. Those
*resolved* values now land in `job_run_meta.runtime_job_template` and are
returned by `GET /jobs/{id}` and `GET /jobs`. Previously `jobConf` was
transient and only the template with `{{placeholder}}` markers was stored.
Any deployment that passes a token, password or connection string as a job
parameter — a normal use of placeholders — will now write that secret to the
metadata store in cleartext and hand it back to every principal who can read
the job. This is a silent behavior change for existing templates, so it
probably needs at least a redaction hook, or an explicit note in the docs and
release notes.
--
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]
