feat(buildrunner): drop per-call queueName, add Factory#184
Closed
behinddwalls wants to merge 1 commit into
Closed
feat(buildrunner): drop per-call queueName, add Factory#184behinddwalls wants to merge 1 commit into
behinddwalls wants to merge 1 commit into
Conversation
00cfaab to
aa1bd49
Compare
## Summary
### Why?
`BuildRunner.Trigger` took a `queueName` argument that selected the
runner-specific job configuration on every call. That put queue routing
on the hot path and on a single verb, leaving `Status` and `Cancel` to
rediscover the queue from the build ID. The queue belongs at construction
time, not per call.
### What?
- Drop `queueName` from `BuildRunner.Trigger`; the verbs speak only in
builds and changes.
- Add a `Factory` interface (`New(cfg Config) (BuildRunner, error)`) and
a `Config` struct carrying `QueueID`. A runner is bound to its Config at
construction; backends extend `Config` with their own settings.
- noop: add `NewFactory()`; keep `New()`.
- Wire the factory end to end: the build and buildsignal controllers hold
a `buildrunner.Factory` and build a runner per queue via
`New(Config{QueueID: batch.Queue})`. buildsignal loads the batch to
resolve the queue (TODO: denormalize queue onto the Build). The example
orchestrator wires `buildnoop.NewFactory()`.
- Docs: BuildRunner is no longer described as a "singleton" but must stay
safe for concurrent use. Updated the RFC (Construction section) and
README.
Caching of runners per queue is intentionally omitted for now.
## Test Plan
✅ `make mocks`, `make gazelle`, `make tidy`, `make fmt`
✅ `bazel build //...`
✅ `make test` (unit), incl. buildrunner + build + buildsignal
aa1bd49 to
30458c9
Compare
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
Why?
BuildRunner.Triggertook aqueueNameargument that selected therunner-specific job configuration on every call. That put queue routing
on the hot path and on a single verb, leaving
StatusandCanceltorediscover the queue from the build ID. The queue belongs at construction
time, not per call.
What?
queueNamefromBuildRunner.Trigger; the verbs speak only inbuilds and changes.
Factoryinterface (New(cfg Config) (BuildRunner, error)) anda
Configstruct carryingQueueID. A runner is bound to its Config atconstruction; backends extend
Configwith their own settings.NewFactory(); keepNew().a
buildrunner.Factoryand build a runner per queue viaNew(Config{QueueID: batch.Queue}). buildsignal loads the batch toresolve the queue (TODO: denormalize queue onto the Build). The example
orchestrator wires
buildnoop.NewFactory().safe for concurrent use. Updated the RFC (Construction section) and
README.
Caching of runners per queue is intentionally omitted for now.
Test Plan
✅
make mocks,make gazelle,make tidy,make fmt✅
bazel build //...✅
make test(unit), incl. buildrunner + build + buildsignal