feat(stovepipe): add and wire the buildsignal stage controller#409
feat(stovepipe): add and wire the buildsignal stage controller#409roychying wants to merge 2 commits into
Conversation
roychying
changed the title
roychying
changed the title
7459fe1 to
63562b5
Compare
| // PollDelayAcceptedMs is the delay before the next Status call while the | ||
| // build is queued by the runner but has not started executing. | ||
| PollDelayAcceptedMs int64 = 5000 | ||
| // PollDelayRunningMs is the delay before the next Status call while the | ||
| // build is executing. | ||
| PollDelayRunningMs int64 = 2000 | ||
| ) |
There was a problem hiding this comment.
i think this needs to be configurable and come via controller inputs eventially, can you add TODO to do so
| buildRunner, err := c.buildRunners.For(buildrunner.Config{QueueName: request.Queue}) | ||
| if err != nil { | ||
| // A queue with no registered builder is a config error. | ||
| return fmt.Errorf("BuildSignalController failed to resolve build runner for queue %s: %w", request.Queue, err) |
There was a problem hiding this comment.
BuildSignalController marker? can we remove those?
There was a problem hiding this comment.
same as this thread: #407 (comment)
happy to refactor all in another pr
| // loadRequest returns the request for id. A miss here is almost certainly a | ||
| // lagging read behind the Build that referenced it, so it is retryable; a | ||
| // genuinely orphaned Build still dead-letters at MaxAttempts. | ||
| func (c *Controller) loadRequest(ctx context.Context, id string) (entity.Request, error) { |
There was a problem hiding this comment.
i see it being duplicated between build and buildsignal
There was a problem hiding this comment.
Maybe we should align on this more generally. Do we want to make each controller own its own logic for fetching required entities, or have some kind of intermediate later - e.g. how we typically put all database / retrieval operations behind a repository package in our internal services and it just returns the entity.
There was a problem hiding this comment.
It looks like in submitqueue most controllers have been handling it themselves so far but it could lead to duplication.
There was a problem hiding this comment.
will introduce a shared helper in stovepipe for all loadX pattern
| if err == nil { | ||
| return got, nil | ||
| } | ||
| if errors.Is(err, storage.ErrNotFound) { |
There was a problem hiding this comment.
same as other PR, why do we need to handle not found?
There was a problem hiding this comment.
I've noticed agents tend to add this as a defense against DB replication lag, maybe won't happen much in the real world but feels reasonable to guard against.
There was a problem hiding this comment.
it's true. also discussed in build rfc pr: #336 (comment). but it's also making sense to remove it as well.. will do
3 participants
What?
Adds
stovepipe/controller/buildsignalplus the newRecordqueue message/proto andTopicKeyRecord.Processconsumes aBuildSignal(build id), loadsBuild+Request, no-ops if the request is terminal, pollsbuildRunner.Status, persists a real status transition via CAS (BuildStore.Update) with terminal status write-once (a later poll of a flaky backend can never overwrite an already-committed terminal status), and either reschedules itself viaPublishAfter(5s whileaccepted, 2s whilerunning) or, once terminal, publishes torecord. Wires the controller intomain.goand registers thebuildsignal(subscribed) andrecord(publish-only, no consumer yet) topics.Test Plan
make build,make test- all clean.Issue