feat: migrate to Connect-RPC; new wire path /jobs.v2.JobsService/#147
Merged
Conversation
Switches the jobs plugin from net/rpc + goridge codec to Connect-RPC.
Plugin.RPC() now returns the generated jobsV2connect.NewJobsServiceHandler
mounted at /jobs.v2.JobsService/ on the rpc plugin's HTTP/2 mux.
The 8 wire methods are reshaped from (req, *resp) error to the Connect
handler interface:
Push, PushBatch, Pause, Resume, List, Declare, Destroy, GetStats
Notable shape changes:
- Push now takes PushRequest { Job job = 1; }; the legacy runtime guard
for "exactly one job" inside PushBatchRequest is gone (proto enforces).
- Stat -> GetStats (proto rename to avoid collision with the Stat message).
- List, GetStats take google.protobuf.Empty.
- Errors wrap via connect.NewError(connect.CodeInternal, ...) for proper
gRPC status codes; OTEL spans + per-method mutex preserved.
Test helpers swapped to an h2c Connect client (jobsV2connect.JobsServiceClient)
talking to the rpc plugin's HTTP/2 cleartext listener at 127.0.0.1:6001.
Deps:
- api-go v6.0.0-beta.4 -> v6.0.0-beta.5 (new JobsService bindings)
- goridge v4.0.0-beta.1 -> v4.0.0-beta.2 (pkg/rpc removed)
- rpc/v6 v6.0.0-beta.3 -> v6.0.0-beta.4 (Connect-based RPCer interface)
- + connectrpc.com/connect, + golang.org/x/net (h2c client in tests)
Breaking changes:
- Wire protocol changes from goridge codec to Connect/gRPC. Downstream
consumers must use jobsV2connect.NewJobsServiceClient (or any gRPC
client) against the new path. The PHP goridge client migrates
separately on its own timeline.
- Push/PushBatch: thread inbound handler ctx through rpcContextFromJob/ rpcContextFromJobs/rpcContextFromHeaders so client cancellation and deadlines propagate. The job-header trace context still overrides the span parent (PHP-side propagation preserved); only the propagator's parent argument changes. - Pause/Resume: wrap errors with errors.E(op, err) for consistency with other methods (rpc_pause, rpc_resume ops). - Tests (helpers): t.Cleanup(httpc.CloseIdleConnections) on each NewJobsClient to avoid leaking idle HTTP/2 connections across tests. - rpc_test.go: pass t.Context() to the 5 helper call sites for the new signature.
- Destroy: wrap errg.Wait() error with errors.E(op, err) for parity with the other 7 methods. - Push: add unit tests covering nil-job and empty-ID branches (the proto-shape replacement for the legacy len(req.Batch) != 1 guard). - Trim WHAT-restating docstrings on Plugin.RPC, rpc.from, rpc.Push, rpc.Declare, helpers.NewJobsClient. Tighten the rpcContextFromJobs doc to the single non-obvious WHY (parent-ctx layering).
When a pipeline's priority field is present but non-numeric, strconv.Atoi left prInt at its zero value, which Plugin.Declare then saved as the pipeline's priority — silently coercing user input to 0 despite the "continue with default priority" comment that implies 10 (the same fallback used for missing priority on the line above). Now the parse-error branch resets prInt to 10 to match the comment, and the log line surfaces the chosen fallback so operators can see when their priority value was rejected.
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes Plugin.Declare to honor the documented default pipeline priority when a provided priority value is non-numeric, preventing silent fallback to 0 and improving operator visibility via logging.
Changes:
- Reset
prIntto the documented default (10) whenstrconv.Atoifails while parsing pipelinepriority. - Augment the error log to include the selected fallback priority value.
- Clarify the inline comment to reflect the intended default-on-parse-error behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+557
to
+562
| // save priority as int64; fall back to the documented default on parse error. | ||
| pr := pipeline.String(priority, "10") | ||
| prInt, err := strconv.Atoi(pr) | ||
| if err != nil { | ||
| // we can continue with a default priority | ||
| p.log.Error(priority, "error", err) | ||
| p.log.Error(priority, "error", err, "fallback", 10) | ||
| prInt = 10 |
rustatian
changed the title
- pipeline.String fallback now formats defaultPriority instead of "10" - log "fallback" field uses defaultPriority directly - reset on Atoi failure uses defaultPriority Also switches strconv.Atoi → ParseInt to drop the redundant int64 cast, matching the int64 type defaultPriority is declared as and the int64 the pipeline ultimately stores.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Switches the jobs plugin's RPC layer from net/rpc + goridge codec to Connect-RPC over HTTP/2.
Plugin.RPC()now returns the generatedjobsV2connect.NewJobsServiceHandler, which the rpc plugin (≥ v6.0.0-beta.4) mounts at/jobs.v2.JobsService/on its HTTP/2 mux.The 8 wire methods are reshaped from net/rpc's
(req, *resp) errorto the Connect handler interface. Notable contract changes:Pushnow takesPushRequest { Job job = 1; }— proto-shape enforces single-job, the legacylen(req.Batch) != 1runtime guard is gone.Stat→GetStats(proto rename to avoid same-package collision with theStatmessage).List,GetStatstakegoogle.protobuf.Empty.connect.NewError(connect.CodeInternal, errors.E(op, err)); OTEL spans + per-method mutex preserved.Test helpers swapped to an h2c Connect client (
jobsV2connect.JobsServiceClient) talking to the rpc plugin's HTTP/2 cleartext listener at127.0.0.1:6001. Exportedhelpers.NewJobsClient(t, address)for sibling test packages. Variadic-pipes copies useslices.Clone(pipes)(Go 1.21+ idiom).Also fixes a pre-existing
Plugin.Declarepriority bug (surfaced during the silent-failure-hunter review and folded into this PR): when a pipeline'spriorityfield was non-numeric,strconv.Atoifailed andprIntwas silently saved as0rather than the documented default of10. Now the parse-error branch resetsprInt = 10and the log line surfaces the chosen fallback.Deps:
api-go/v6 v6.0.0-beta.4 → v6.0.0-beta.5(newJobsServicebindings)goridge/v4 v4.0.0-beta.1 → v4.0.0-beta.2(pkg/rpcremoved)rpc/v6 v6.0.0-beta.3 → v6.0.0-beta.4(Connect-basedRPCerinterface)+ connectrpc.com/connect,+ golang.org/x/net(h2c transport for tests)Breaking changes
127.0.0.1:6001) to Connect/gRPC over HTTP/2 at/jobs.v2.JobsService/<Method>. Downstream Go consumers must usejobsV2connect.NewJobsServiceClient(or any gRPC client). The PHP goridge client migrates separately on its own timeline.Pushrequest type changed fromPushBatchRequest(1-element) toPushRequest { Job }.StatRPC method renamed toGetStats.