Add external_job_uri as a first-class proto field#4823
Add external_job_uri as a first-class proto field#4823dejanzele merged 2 commits intoarmadaproject:masterfrom
Conversation
Greptile SummaryThis PR promotes Confidence Score: 5/5Safe to merge — all three compat scenarios are handled, field numbers are correct, and test coverage is thorough. No P0 or P1 findings. Proto field numbers don't conflict with reserved fields. The proto-wins/annotation-fallback logic is consistent in both the server conversion and the ingester. The previous concern about mutating the caller's annotations map is resolved. Test suite covers all edge cases including nil annotations. No files require special attention.
|
| Filename | Overview |
|---|---|
| pkg/api/submit.proto | Adds external_job_uri = 13 to JobSubmitRequestItem; field 12 is reserved so 13 is the correct next number. No conflicts. |
| pkg/armadaevents/events.proto | Adds external_job_uri = 16 to SubmitJob; fields 1, 13, 15 are reserved, so 16 is correct. No conflicts. |
| internal/server/submit/conversion/conversions.go | Correctly resolves externalJobUri by preferring the proto field and falling back to the annotation; no map mutation; sets the resolved value on SubmitJob.ExternalJobUri. |
| internal/lookoutingester/instructions/instructions.go | Same proto-field-first / annotation-fallback logic as the server conversion; includes truncation to maxAnnotationValLen before storing. |
| internal/server/submit/conversion/conversions_test.go | Adds TestExternalJobUri covering proto-wins, annotation-fallback, neither-set, and nil-annotations cases — good coverage. |
| internal/lookoutingester/instructions/instructions_test.go | Adds TestExternalJobUriProtoFieldPreferred with identical three-case coverage; existing TestConvert updated to use the named constant. |
| internal/common/constants/constants.go | Adds ExternalJobUriAnnotation constant to replace the magic string "armadaproject.io/externalJobUri" used in four places; clearly documented as legacy. |
| client/rust/src/builder.rs | Adds external_job_uri field and builder method to the Rust JobRequestItemBuilder; correctly wired into both build() and build_from_pod_spec() paths. |
Sequence Diagram
sequenceDiagram
participant C as Client
participant S as Server (conversions.go)
participant P as Pulsar
participant I as Ingester (instructions.go)
participant DB as Lookout DB
C->>S: JobSubmitRequestItem\n(ExternalJobUri proto field OR annotation)
Note over S: Prefer proto field\nFall back to annotation
S->>P: SubmitJob event\n(ExternalJobUri field set)
P->>I: Consume SubmitJob event
Note over I: Prefer ExternalJobUri field\nFall back to annotation\n(for pre-field events)
I->>DB: CreateJobInstruction\n(external_job_uri column)
Reviews (9): Last reviewed commit: "Merge branch 'master' into external-job-..." | Re-trigger Greptile
4e76cb9 to
04b14e8
Compare
The external job URI is already treated as special in Armada (dedicated DB column, dedicated query API endpoint), but clients have to know the magic annotation key "armadaproject.io/externalJobUri" to use it. This makes the field undiscoverable and means clients get no help from type checking or generated docs. This adds external_job_uri as a proper proto field on JobSubmitRequestItem and SubmitJob, so clients can set it directly. The server resolves the value from the proto field first and falls back to the annotation for backward compatibility. The value is mirrored into the annotation on the outgoing event so older ingesters that only read the annotation continue to work during rolling deploys. The Airflow operator is updated to use the proto field when available (guarded by hasattr for older client library versions) while continuing to set the annotation for backward compat with older servers. Signed-off-by: Dejan Zele Pejchev <pejcev.dejan@gmail.com>
04b14e8 to
fe6372c
Compare
Summary
The external job URI already gets special treatment in Armada (its own DB column, its own query API endpoint), but clients have to know the magic annotation key
armadaproject.io/externalJobUrito set it. There is nothing in the API that tells you this annotation exists or matters.This promotes it to a proper proto field on
JobSubmitRequestItemandSubmitJobso clients get autocompletion, type checking, and generated docs for free.Backward compatibility
Changes
pkg/api/submit.proto,pkg/armadaevents/events.proto- Addedexternal_job_urifieldinternal/server/submit/conversion/conversions.go- Resolve from proto field first, fall back to annotation, mirror into annotationinternal/lookoutingester/instructions/instructions.go- Read proto field first, fall back to annotationinternal/common/constants/constants.go- Shared constant for the annotation keyclient/rust/src/builder.rs- Added field to Rust client builderNo DB migration needed since the
external_job_uricolumn already exists.Test plan
go test ./internal/server/submit/conversion/...passesgo test ./internal/lookoutingester/instructions/...passesmage lintCheckpassesmage buildPythongenerates the field in the Python clientcargo buildandcargo testpass for Rust client