Skip to content

Commit 5cf9d52

Browse files
authored
Merge branch 'main' into wua/status-list-api-rfc
2 parents 593cda9 + 87edf75 commit 5cf9d52

101 files changed

Lines changed: 2516 additions & 387 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

CLAUDE.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,7 @@ Rule of thumb: if you're about to add a `NewFactory()` or a `map[queue]impl` und
141141
Common over-constraints to avoid:
142142
- **Batch atomicity** (multi-row inserts as one transaction) — many KV stores can't do this. Prefer single-record primitives + caller loops + idempotency-on-retry.
143143
- **Multi-key queries** (`WHERE x IN (...)`) — fine in SQL, awkward elsewhere. Prefer per-key reads.
144+
- **Query-by-attribute / secondary indexes** (`WHERE attr = ?`, `ListByX(attr)`) — a plain KV store cannot look up by anything but the primary key. The mechanical smell test: **if a schema change adds a `KEY idx_*` to make a store method viable, the contract has stopped being get/put-by-key.** Instead, derive the primary key from the composite identity the caller already holds (e.g. `{parentID}/{hash(child identity)}`), and remember that domain state is often already the index — an entity that references its children (a tree listing its paths) enumerates their keys for free. When neither applies, a genuinely needed reverse lookup gets its own first-class mapping store keyed by the attribute — in the KV space that is the mechanism, not a workaround. See the decision path in [submitqueue/extension/storage/README.md](submitqueue/extension/storage/README.md#key-value-contract).
144145
- **Server-side filters** (joins, sub-queries, complex predicates) — push filtering and aggregation to the caller; keep the store responsible only for "get/put by key" semantics.
145146
- **Transactions across entities** — virtually no distributed store offers this. Use eventual consistency + idempotency.
146147
- **Strict ordering / exactly-once** in messaging — most queues are at-least-once with best-effort ordering. Make consumers idempotent.

Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -364,7 +364,7 @@ local-stovepipe-stop: ## Stop the Stovepipe service
364364

365365
mocks: ## Generate mock files using mockgen
366366
@echo "Generating mocks..."
367-
@$(BAZEL) run @rules_go//go -- generate ./submitqueue/extension/storage/... ./submitqueue/extension/buildrunner/... ./submitqueue/extension/changeprovider/... ./platform/extension/counter/... ./platform/extension/messagequeue/... ./submitqueue/extension/queueconfig/... ./submitqueue/extension/mergechecker/... ./submitqueue/extension/pusher/... ./submitqueue/extension/scorer/... ./submitqueue/extension/conflict/... ./platform/consumer/... ./stovepipe/extension/storage/... ./stovepipe/extension/sourcecontrol/...
367+
@$(BAZEL) run @rules_go//go -- generate ./submitqueue/extension/storage/... ./submitqueue/extension/buildrunner/... ./submitqueue/extension/changeprovider/... ./platform/extension/counter/... ./platform/extension/messagequeue/... ./submitqueue/extension/queueconfig/... ./submitqueue/extension/mergechecker/... ./submitqueue/extension/pusher/... ./submitqueue/extension/scorer/... ./submitqueue/extension/conflict/... ./submitqueue/extension/validator/... ./platform/consumer/... ./stovepipe/extension/storage/... ./stovepipe/extension/sourcecontrol/...
368368
@echo "Mocks generated successfully!"
369369

370370
proto: ## Generate protobuf files from .proto definitions

api/base/change/proto/change.proto

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,14 +29,15 @@ option java_package = "com.uber.submitqueue.base.change";
2929
// Stovepipe, and other repo-local domains — the proto-level analog of the
3030
// platform/base/change Go entity. Domains import it rather than redefining their own.
3131
message Change {
32-
// URIs identifying the change(s) (RFC 3986 compliant). The scheme identifies the
33-
// change provider, and the path contains provider-specific resource identifiers.
32+
// URIs identifying the change(s) (RFC 3986 compliant): scheme://<host[:port]>/<path>.
33+
// The scheme identifies the change provider, the authority is the provider instance
34+
// the change lives on, and the path contains provider-specific resource identifiers.
3435
//
3536
// Supported by default (other providers can be added):
36-
// GitHub PR: "github://<org>/<repo>/pull/<pr>/<head_commit_sha>"
37-
// ("ghe"/"ghes" schemes for GitHub Enterprise)
38-
// git commit: "git://<remote>/<repo>/<ref>/<commit_sha>"
39-
// (<ref> is a fully-qualified, percent-encoded git ref)
37+
// GitHub PR: "github://<host[:port]>/<org>/<repo>/pull/<pr>/<head_commit_sha>"
38+
// Phabricator Diff: "phab://<host[:port]>/D<revision>/<diff>"
39+
// git commit: "git://<host[:port]>/<repo>/<ref>/<commit_sha>"
40+
// (<ref> is a fully-qualified, percent-encoded git ref)
4041
//
4142
// The commit SHA must be the full 40-character lowercase hex SHA; abbreviated
4243
// SHAs are rejected because downstream staleness checks compare by strict equality.

api/base/change/protopb/change.pb.go

Lines changed: 7 additions & 6 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

api/runway/messagequeue/merge_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,12 @@ func TestMergeRequestRoundTrip(t *testing.T) {
3333
Steps: []*MergeStep{
3434
{
3535
StepId: "queue-a/1",
36-
Changes: []*changepb.Change{{Uris: []string{"github://uber/repo/pull/1/0123456789abcdef0123456789abcdef01234567"}}},
36+
Changes: []*changepb.Change{{Uris: []string{"github://github.example.com/uber/repo/pull/1/0123456789abcdef0123456789abcdef01234567"}}},
3737
Strategy: strategypb.Strategy_REBASE,
3838
},
3939
{
4040
StepId: "queue-a/2",
41-
Changes: []*changepb.Change{{Uris: []string{"github://uber/repo/pull/2/89abcdef0123456789abcdef0123456789abcdef"}}},
41+
Changes: []*changepb.Change{{Uris: []string{"github://github.example.com/uber/repo/pull/2/89abcdef0123456789abcdef0123456789abcdef"}}},
4242
Strategy: strategypb.Strategy_MERGE,
4343
},
4444
},

doc/rfc/change-uri.md

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
# Change URIs
2+
3+
A change URI is the system-wide identity of a code change — a Pull Request, a Phabricator Diff, or a git ref/commit. It is minted by the client at submission, validated at the gateway, and flows as an opaque string through the shared `Change` wire contract (`api/base/change`), cross-domain queue payloads, and storage, where it is a primary-key column and correlation key. A change URI must therefore be globally unambiguous on its own: interpretable without knowing which queue carried it or how any backend is wired.
4+
5+
## Shape
6+
7+
Every change URI is an RFC 3986 URI of the form `scheme://{host[:port]}/{path}`, with a uniform division of labor:
8+
9+
- **scheme** — the provider *model*: how to parse the path and which extension family (change provider, merge checker, pusher) can act on it. One scheme per model — deployment flavors of the same model (github.com vs. GitHub Enterprise) do **not** get their own schemes, because the flavor is derivable from the host and two spellings for one instance would break identity.
10+
- **authority** — the provider *instance*: the `host[:port]` the change lives on. Mandatory.
11+
- **path** — the change within that instance, pinned to an exact code state (head SHA or diff ID), so staleness is detectable by comparing the pin against the provider's current state.
12+
13+
## Formats
14+
15+
| Provider | Format | Example |
16+
|---|---|---|
17+
| GitHub PR | `github://{host[:port]}/{org}/{repo}/pull/{pr}/{head_sha}` | `github://github.uberinternal.com/uber/submitqueue/pull/123/c3a4…89ab` |
18+
| Phabricator Diff | `phab://{host[:port]}/D{revision}/{diff}` | `phab://phabricator.example.com/D12345/67890` |
19+
| git ref/commit | `git://{host[:port]}/{repo}/{ref}/{sha}` | `git://git.example.com:9418/uber/mono/refs%2Fheads%2Fmain/c3a4…89ab` |
20+
21+
Path rules per provider:
22+
23+
- **GitHub**`{org}` may be a nested path (`uber/frontend`); the literal `pull` segment separates it from the PR number, mirroring the real PR URL layout so URIs are built by substitution, not reshaping. `{head_sha}` is the PR's head commit at submission time.
24+
- **Phabricator**`D{revision}` is the logical review (stable across updates); `{diff}` is the uploaded patch version that pins the exact code state, analogous to GitHub's head SHA. Both are positive integers without leading zeros.
25+
- **git**`{repo}` is the repository path on the remote and may contain slashes; `{ref}` is a fully-qualified git ref (`refs/heads/main`, `refs/tags/v1.0`), percent-encoded so it occupies a single path segment; `{sha}` is a commit that ref has pointed to.
26+
27+
## Canonical form
28+
29+
URIs are compared as opaque strings everywhere (primary keys, claim lookups, staleness checks), so exactly one spelling per change is valid. Parsers **validate the canonical form and reject everything else — they never normalize**, because normalization applied at one entry point and skipped at another lets two spellings of one change into the system.
30+
31+
- **Host** — required, non-empty, lowercase (DNS is case-insensitive, so case variants would alias one instance into many identities). Uppercase is rejected, not folded.
32+
- **Port** — optional, digits only, verbatim when present. Custom schemes have no registered default port, so there is nothing to strip; omit it unless the backend listens on a non-standard one.
33+
- **Commit SHAs** — the full 40-character lowercase hex form. Abbreviated or uppercase SHAs are rejected, not expanded or folded.
34+
- **All other path segments** — verbatim. Org, repo, and ref segments live in namespaces that are case-sensitive (git refs, repository paths on a git remote) or provider-canonical (GitHub resolves org/repo case-insensitively, but each repo has one canonical casing and uppercase is legal — the parser cannot know which). Folding their case would silently point the identity at a different resource; canonical casing here is the provider's to enforce, at the point where the provider is consulted.
35+
- **Round-trip** — parsing a valid URI and re-serializing the parsed form yields the input byte-for-byte.
36+
37+
Parsing is delegated to `net/url`, which handles `host:port` splitting, bracketed IPv6 hosts, and percent-encoding correctly.
38+
39+
## Rejected alternatives
40+
41+
- **Host out-of-band in queue config.** Conflates identity with routing: the meaning of a stored primary-key value must not depend on deployment wiring, and the shared contract must be interpretable by every domain that imports it.
42+
- **Per-flavor schemes** (`ghe://`, `ghes://`). Redundant with the authority, and an open-ended enum baked into parsers and routing — a new instance should be configuration, not code. The flavors share one PR model and one API surface; what does differ per instance (API base path, version skew) is wiring config on the client for that host, never identifier grammar.
43+
- **The provider's web URL as identity** (`https://github.com/uber/repo/pull/123`). Human-facing URLs don't uniformly pin the code state, vary with provider UI cosmetics, and hand our identity grammar to a third party. Custom schemes keep the grammar strict and ours.

doc/rfc/index.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,12 @@ Design documents and technical proposals, grouped by scope. Shared/cross-cutting
66

77
- [SQL-Based Distributed Queue](sql-queue-rfc.md) - MySQL-based distributed message queue with partition leasing and at-least-once delivery (used by SubmitQueue, Stovepipe, and other repo-local services)
88
- [Message Queue Contract](messagequeue-contract.md) - How queue payloads are defined (Protobuf, serialized as protobuf JSON), located by audience (external in `api/{domain}/messagequeue/`, internal in `{domain}/core/messagequeue/`), bound to topics (the `topics` proto option), and enforced by Bazel visibility
9+
- [Change URIs](change-uri.md) - Identity of a code change: `scheme://{host[:port]}/{path}` per provider (GitHub PR, Phabricator Diff, git ref/commit) and canonical-form rules
910

1011
## SubmitQueue
1112

1213
- [Orchestrator Workflow](submitqueue/workflow.md) - Queue-driven controller pipeline from gateway entry through batching, scoring, build, merge, and conclude
14+
- [Gateway History APIs](submitqueue/history-api.md) - Request lifecycle history exposed through separate request ID and change ID endpoints
1315
- [Build Runner](submitqueue/build-runner.md) - Vendor-agnostic BuildRunner interface, provider-neutral BuildStatus lifecycle, and how the orchestrator wires it into the build stage
1416
- [Extension Contract](submitqueue/extension-contract.md) - When extensions take orchestrator identity (request/batch) and resolve granular content themselves vs. take controller-resolved data; revises the BuildRunner base/head contract
1517
- [Gateway Status and List APIs](submitqueue/status-list-api.md) - Gateway-owned request context, materialized current status, sqid or change-URI status lookup, and queue admission listing

doc/rfc/stovepipe/steps/process.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,7 @@ Transitions use the repo's optimistic-locking pattern: compute `newVersion = old
205205

206206
New key/value-shaped operations (single-key reads/writes, no server-side filtering or aggregation):
207207

208-
- **`QueueStore`** (new): `GetOrCreate(ctx, name, defaults)`, `Get(ctx, name)`, and `Update(ctx, queue, oldVersion, newVersion)` (CAS). Ingest `GetOrCreate`s and CASes `latest_request_seq`; `process` CASes `in_flight_count`; `record` CASes `last_green_uri` + `in_flight_count`.
208+
- **`QueueStore`** (new): `Create(ctx, queue)`, `Get(ctx, name)`, and `Update(ctx, queue, oldVersion, newVersion)` (CAS). Callers orchestrate get-or-create; ingest CASes `latest_request_seq`; `process` CASes `in_flight_count`; `record` CASes `last_green_uri` + `in_flight_count`.
209209
- **`RequestStore`**: no new methods — the added `Request` fields ride the existing `Create`/`Update` CAS.
210210

211211
No "list requests by queue/state" query is introduced; coalescing uses the single-row `latest_request_seq` pointer instead, keeping the contract satisfiable by a plain KV backend.

0 commit comments

Comments
 (0)