From 48be60255eaa4c162311aa24957e0223b5bd60da Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Fri, 7 Aug 2026 11:06:10 -0700 Subject: [PATCH] feat(storage): shard every table by queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? An audit of all 22 `schema/*.sql` files found five tables that are not shardable by queue: their primary key does not lead with the queue, so one queue's rows stay reachable through another queue's binding and the tables cannot be split across shards. The five are `speculation_path_set`, `counter`, `request_summary`, `request_log`, and `change_uri_request_mapping`. Three of those — the gateway read-model tables — were declared permanently unshardable by `submitqueue/extension/storage/storage.go` and the schema README, on the grounds that their lookups "start from identifiers that arrive without queue context". This change removes that exception at its source rather than working around it: the gateway read APIs now require the queue alongside the sqid or change URI, so the queue is always available at the call site and no identifier ever has to be parsed to recover one. The five platform `messagequeue` tables are deliberately out of scope. They are a message-queue backend keyed by `(consumer_group, topic, partition_key)`, not a domain table set, and sharding them is a separate problem (a global `AUTO_INCREMENT` offset, and a subscriber-heartbeat table whose fair-leasing logic genuinely needs the whole live subscriber set). ### What? All 17 in-scope tables now lead their primary key with the queue. **Independent re-keys.** `speculation_path_set` moves to `(queue, head)` — a batch ID is unique only within its queue, so the head alone was never a safe key. `counter` moves to `(queue, domain)` and gains the standard extension `Config`/`Factory` shape; per the extension contract the factory *implementations* live in the three service `main.go` files, not under `platform/extension/`. **Minted identifiers are unchanged.** Counter domains simplify to bare `"request"` / `"batch"`, but every emitted ID keeps its exact current format: `{queue}/{seq}`, `{queue}/batch/{n}`, and stovepipe's `request/{queue}/{seq}`. Stovepipe previously reused its counter domain *as* the ID prefix, which would have silently turned `request/queueA/7` into `request/7`; the two are now written independently so they cannot drift into each other. Note that re-keying `counter` restarts every sequence at 1, so its table must be recreated in the same cutover as `request`, `batch`, `request_summary`, and `request_log` — never on its own, or freshly minted sqids will collide with surviving rows. **Queue required in the gateway contract.** `CancelRequest`, `GetRequestSummaryByIDRequest`, `GetRequestSummaryByChangeURIRequest`, `GetRequestHistoryByIDRequest`, and `GetRequestHistoryByChangeURIRequest` each gain a `queue` field, validated on entry through the existing `validateQueueIdentifier`. Two behaviour changes follow. The change-URI lookups are now scoped to one queue, so the same URI landed into several queues needs one call per queue. And `Cancel` no longer overrides the caller's queue with the stored one — a mismatched queue yields `NotFound`, since a sqid is simply not resolvable outside its own queue. **Read-model tables re-keyed and folded in.** `request_summary`, `request_log`, and `change_uri_request_mapping` move to queue-leading keys and join the `Storage` aggregate; the `SetGlobalStores` seam and the "deliberately not part of this aggregate" carve-out are deleted. The queue travels as an explicit field on `entity.RequestLog` and `entity.RequestURI`, stamped by producers that already know it — no ID parsing is introduced anywhere. **Enforcement.** A new `//tool/linter/queueshard` walks the schema directories and fails if any primary key does not lead with the queue, or if any secondary index does not — a non-queue-leading index would reintroduce exactly the cross-queue access path the primary key just closed. It is wired into `make lint`. Schema changes are clean recreates, not online migrations, on the schema README's statement that these tables are created empty at rollout and never backfilled. ## Test Plan - ✅ `make test` — 93/93 pass, including the new linter's own tests - ✅ `bazel test //test/integration/...` — 8/8 suites pass against real MySQL, including new cross-queue isolation coverage in the counter, storage, request-URI and request-log contract suites - ✅ `bazel test //test/e2e/...` — both suites pass against the full Docker Compose stack (land → landed, plus status/history/cancel/list through the queue-scoped APIs) - ✅ `make lint` — fmt, license headers, and `queueshard` ("All 17 tables are shardable by queue") - ✅ `make fmt` / `make gazelle` / `make mocks` / `make tidy` — no drift The e2e and image-building integration targets need `--sandbox_writable_path=$HOME/.docker` when run locally; `make e2e-test` does not pass it, which is a pre-existing local-only issue unrelated to this change. --- Makefile | 5 +- api/submitqueue/gateway/proto/gateway.proto | 15 ++ api/submitqueue/gateway/protopb/gateway.pb.go | 85 ++++++-- .../gateway/protopb/gateway.pb.yarpc.go | 137 ++++++------ doc/rfc/submitqueue/history-api.md | 21 +- doc/rfc/submitqueue/status-list-api.md | 12 +- platform/extension/counter/README.md | 35 ++-- platform/extension/counter/counter.go | 24 ++- platform/extension/counter/mock/BUILD.bazel | 5 +- .../extension/counter/mock/counter_mock.go | 40 ++++ platform/extension/counter/mysql/counter.go | 20 +- .../counter/mysql/schema/counter.sql | 5 +- service/stovepipe/server/BUILD.bazel | 1 + service/stovepipe/server/main.go | 39 +++- .../submitqueue/gateway/server/BUILD.bazel | 1 + service/submitqueue/gateway/server/main.go | 47 +++-- .../gateway/server/mapper/cancel.go | 1 + .../gateway/server/mapper/cancel_test.go | 12 +- .../gateway/server/mapper/request_history.go | 4 +- .../server/mapper/request_history_test.go | 8 +- .../gateway/server/mapper/request_summary.go | 4 +- .../server/mapper/request_summary_test.go | 8 +- .../orchestrator/server/BUILD.bazel | 1 + .../submitqueue/orchestrator/server/main.go | 20 +- stovepipe/controller/BUILD.bazel | 1 + stovepipe/controller/ingest.go | 26 ++- stovepipe/controller/ingest_test.go | 13 +- submitqueue/core/request/log.go | 5 +- submitqueue/core/request/log_test.go | 16 +- submitqueue/core/request/materializer.go | 45 ++-- submitqueue/core/request/materializer_test.go | 9 +- submitqueue/core/request/terminate.go | 2 +- submitqueue/entity/request_history.go | 6 + submitqueue/entity/request_log.go | 7 +- submitqueue/entity/request_log_test.go | 2 +- submitqueue/entity/request_summary.go | 9 + submitqueue/entity/speculation.go | 7 +- .../extension/storage/mock/storage_mock.go | 42 ++++ .../storage/mysql/request_log_store.go | 36 ++-- .../storage/mysql/request_log_store_test.go | 29 +-- .../storage/mysql/request_summary_store.go | 37 ++-- .../mysql/request_summary_store_test.go | 53 ++--- .../storage/mysql/request_uri_store.go | 27 ++- .../storage/mysql/request_uri_store_test.go | 31 +-- .../extension/storage/mysql/schema/README.md | 14 +- .../schema/change_uri_request_mapping.sql | 7 +- .../storage/mysql/schema/request_log.sql | 6 +- .../storage/mysql/schema/request_summary.sql | 7 +- .../mysql/schema/speculation_path_set.sql | 6 +- .../mysql/speculation_path_set_store.go | 49 +++-- .../mysql/speculation_path_set_store_test.go | 77 ++++--- .../extension/storage/mysql/storage.go | 56 +++-- .../extension/storage/mysql/storage_test.go | 8 +- .../storage/speculation_path_set_store.go | 10 +- submitqueue/extension/storage/storage.go | 14 +- submitqueue/gateway/controller/cancel.go | 40 ++-- submitqueue/gateway/controller/cancel_test.go | 24 +-- submitqueue/gateway/controller/land.go | 28 ++- submitqueue/gateway/controller/land_test.go | 61 ++++-- .../gateway/controller/log/log_test.go | 15 +- .../gateway/controller/request_history.go | 39 ++-- .../controller/request_history_test.go | 22 +- .../gateway/controller/request_summary.go | 39 ++-- .../controller/request_summary_test.go | 22 +- .../controller/storage_fixture_test.go | 44 +++- .../orchestrator/controller/batch/BUILD.bazel | 1 + .../orchestrator/controller/batch/batch.go | 20 +- .../controller/batch/batch_test.go | 23 +- .../mergeconflictsignal.go | 4 +- .../orchestrator/controller/start/start.go | 2 +- submitqueue/orchestrator/pipeline.go | 4 +- test/e2e/submitqueue/harness_test.go | 72 ++++--- test/e2e/submitqueue/suite_test.go | 56 ++--- .../extension/counter/mysql/BUILD.bazel | 1 + .../extension/counter/mysql/counter_test.go | 18 +- test/integration/extension/counter/suite.go | 51 ++++- .../extension/storage/mysql/storage_test.go | 5 +- .../submitqueue/extension/storage/suite.go | 139 +++++++++--- .../submitqueue/gateway/suite_test.go | 36 ++-- tool/linter/queueshard/BUILD.bazel | 24 +++ tool/linter/queueshard/main.go | 198 ++++++++++++++++++ tool/linter/queueshard/main_test.go | 148 +++++++++++++ 82 files changed, 1703 insertions(+), 640 deletions(-) create mode 100644 tool/linter/queueshard/BUILD.bazel create mode 100644 tool/linter/queueshard/main.go create mode 100644 tool/linter/queueshard/main_test.go diff --git a/Makefile b/Makefile index d4dfed598..d40c57308 100644 --- a/Makefile +++ b/Makefile @@ -172,7 +172,7 @@ integration-test-submitqueue-orchestrator: ## Run Orchestrator integration tests license-fix: ## Add missing license headers to source files @$(BAZEL) run //tool/linter/licenseheader -- --fix -lint: lint-fmt lint-license ## Run all linters +lint: lint-fmt lint-license lint-queue-shard ## Run all linters @echo "All lint checks passed." lint-fmt: fmt ## Check code formatting (fails if unformatted) @@ -182,6 +182,9 @@ lint-fmt: fmt ## Check code formatting (fails if unformatted) lint-license: ## Check license headers on all source files @$(BAZEL) run //tool/linter/licenseheader -- --check +lint-queue-shard: ## Check every table's primary key leads with the queue column + @$(BAZEL) run //tool/linter/queueshard + local-submitqueue-clean: ## Stop and remove all local services, volumes, and images @echo "Cleaning all services and data..." @$(COMPOSE) -f $(COMPOSE_FILE) -p $(SUBMITQUEUE_LOCAL_PROJECT) down -v --rmi local diff --git a/api/submitqueue/gateway/proto/gateway.proto b/api/submitqueue/gateway/proto/gateway.proto index 3c571e803..2a14f359f 100644 --- a/api/submitqueue/gateway/proto/gateway.proto +++ b/api/submitqueue/gateway/proto/gateway.proto @@ -74,6 +74,9 @@ message CancelRequest { string sqid = 1; // Optional human-readable reason for the cancellation. Recorded for observability. string reason = 2; + // Name of the queue processing the request. Required. A sqid is only resolvable within its own queue, + // so naming a queue the request does not belong to is reported as not found. + string queue = 3; } // CancelResponse defines the response to a cancel request. Empty on success. @@ -106,6 +109,9 @@ message RequestSummary { message GetRequestSummaryByIDRequest { // Globally unique identifier for the request. string sqid = 1; + // Name of the queue processing the request. Required. A sqid is only resolvable within its own queue, + // so naming a queue the request does not belong to is reported as not found. + string queue = 2; } // GetRequestSummaryByIDResponse contains the current materialized request view. @@ -118,6 +124,9 @@ message GetRequestSummaryByIDResponse { message GetRequestSummaryByChangeURIRequest { // Exact change URI supplied in a Land request. string change_uri = 1; + // Name of the queue to search. Required. Results are scoped to this queue: a change URI landed + // into several queues is looked up one queue at a time. + string queue = 2; } // GetRequestSummaryByChangeURIResponse contains matching requests newest first. @@ -152,6 +161,9 @@ message ListResponse { message GetRequestHistoryByIDRequest { // Globally unique identifier for the request. string sqid = 1; + // Name of the queue processing the request. Required. A sqid is only resolvable within its own queue, + // so naming a queue the request does not belong to is reported as not found. + string queue = 2; } // HistoryEvent is one retained append-only request-log event. @@ -176,6 +188,9 @@ message GetRequestHistoryByIDResponse { message GetRequestHistoryByChangeURIRequest { // Exact change URI supplied in a Land request. string change_uri = 1; + // Name of the queue to search. Required. Results are scoped to this queue: a change URI landed + // into several queues is looked up one queue at a time. + string queue = 2; } // RequestHistory groups retained events for one request. diff --git a/api/submitqueue/gateway/protopb/gateway.pb.go b/api/submitqueue/gateway/protopb/gateway.pb.go index 61c7328d5..ff5e32a18 100644 --- a/api/submitqueue/gateway/protopb/gateway.pb.go +++ b/api/submitqueue/gateway/protopb/gateway.pb.go @@ -279,7 +279,10 @@ type CancelRequest struct { // Globally unique identifier of the land request to cancel, returned by a prior Land call. Sqid string `protobuf:"bytes,1,opt,name=sqid,proto3" json:"sqid,omitempty"` // Optional human-readable reason for the cancellation. Recorded for observability. - Reason string `protobuf:"bytes,2,opt,name=reason,proto3" json:"reason,omitempty"` + Reason string `protobuf:"bytes,2,opt,name=reason,proto3" json:"reason,omitempty"` + // Name of the queue processing the request. Required. A sqid is only resolvable within its own queue, + // so naming a queue the request does not belong to is reported as not found. + Queue string `protobuf:"bytes,3,opt,name=queue,proto3" json:"queue,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -328,6 +331,13 @@ func (x *CancelRequest) GetReason() string { return "" } +func (x *CancelRequest) GetQueue() string { + if x != nil { + return x.Queue + } + return "" +} + // CancelResponse defines the response to a cancel request. Empty on success. // // A successful response indicates only that the cancellation intent was accepted and enqueued — it does NOT confirm @@ -473,7 +483,10 @@ func (x *RequestSummary) GetMetadata() map[string]string { type GetRequestSummaryByIDRequest struct { state protoimpl.MessageState `protogen:"open.v1"` // Globally unique identifier for the request. - Sqid string `protobuf:"bytes,1,opt,name=sqid,proto3" json:"sqid,omitempty"` + Sqid string `protobuf:"bytes,1,opt,name=sqid,proto3" json:"sqid,omitempty"` + // Name of the queue processing the request. Required. A sqid is only resolvable within its own queue, + // so naming a queue the request does not belong to is reported as not found. + Queue string `protobuf:"bytes,2,opt,name=queue,proto3" json:"queue,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -515,6 +528,13 @@ func (x *GetRequestSummaryByIDRequest) GetSqid() string { return "" } +func (x *GetRequestSummaryByIDRequest) GetQueue() string { + if x != nil { + return x.Queue + } + return "" +} + // GetRequestSummaryByIDResponse contains the current materialized request view. type GetRequestSummaryByIDResponse struct { state protoimpl.MessageState `protogen:"open.v1"` @@ -565,7 +585,10 @@ func (x *GetRequestSummaryByIDResponse) GetRequest() *RequestSummary { type GetRequestSummaryByChangeURIRequest struct { state protoimpl.MessageState `protogen:"open.v1"` // Exact change URI supplied in a Land request. - ChangeUri string `protobuf:"bytes,1,opt,name=change_uri,json=changeUri,proto3" json:"change_uri,omitempty"` + ChangeUri string `protobuf:"bytes,1,opt,name=change_uri,json=changeUri,proto3" json:"change_uri,omitempty"` + // Name of the queue to search. Required. Results are scoped to this queue: a change URI landed + // into several queues is looked up one queue at a time. + Queue string `protobuf:"bytes,2,opt,name=queue,proto3" json:"queue,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -607,6 +630,13 @@ func (x *GetRequestSummaryByChangeURIRequest) GetChangeUri() string { return "" } +func (x *GetRequestSummaryByChangeURIRequest) GetQueue() string { + if x != nil { + return x.Queue + } + return "" +} + // GetRequestSummaryByChangeURIResponse contains matching requests newest first. type GetRequestSummaryByChangeURIResponse struct { state protoimpl.MessageState `protogen:"open.v1"` @@ -794,7 +824,10 @@ func (x *ListResponse) GetNextPageToken() string { type GetRequestHistoryByIDRequest struct { state protoimpl.MessageState `protogen:"open.v1"` // Globally unique identifier for the request. - Sqid string `protobuf:"bytes,1,opt,name=sqid,proto3" json:"sqid,omitempty"` + Sqid string `protobuf:"bytes,1,opt,name=sqid,proto3" json:"sqid,omitempty"` + // Name of the queue processing the request. Required. A sqid is only resolvable within its own queue, + // so naming a queue the request does not belong to is reported as not found. + Queue string `protobuf:"bytes,2,opt,name=queue,proto3" json:"queue,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -836,6 +869,13 @@ func (x *GetRequestHistoryByIDRequest) GetSqid() string { return "" } +func (x *GetRequestHistoryByIDRequest) GetQueue() string { + if x != nil { + return x.Queue + } + return "" +} + // HistoryEvent is one retained append-only request-log event. type HistoryEvent struct { state protoimpl.MessageState `protogen:"open.v1"` @@ -959,7 +999,10 @@ func (x *GetRequestHistoryByIDResponse) GetEvents() []*HistoryEvent { type GetRequestHistoryByChangeURIRequest struct { state protoimpl.MessageState `protogen:"open.v1"` // Exact change URI supplied in a Land request. - ChangeUri string `protobuf:"bytes,1,opt,name=change_uri,json=changeUri,proto3" json:"change_uri,omitempty"` + ChangeUri string `protobuf:"bytes,1,opt,name=change_uri,json=changeUri,proto3" json:"change_uri,omitempty"` + // Name of the queue to search. Required. Results are scoped to this queue: a change URI landed + // into several queues is looked up one queue at a time. + Queue string `protobuf:"bytes,2,opt,name=queue,proto3" json:"queue,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -1001,6 +1044,13 @@ func (x *GetRequestHistoryByChangeURIRequest) GetChangeUri() string { return "" } +func (x *GetRequestHistoryByChangeURIRequest) GetQueue() string { + if x != nil { + return x.Queue + } + return "" +} + // RequestHistory groups retained events for one request. type RequestHistory struct { state protoimpl.MessageState `protogen:"open.v1"` @@ -1284,10 +1334,11 @@ const file_gateway_proto_rawDesc = "" + "\x06change\x18\x02 \x01(\v2\x18.uber.base.change.ChangeR\x06change\x12=\n" + "\bstrategy\x18\x04 \x01(\x0e2!.uber.base.mergestrategy.StrategyR\bstrategy\"\"\n" + "\fLandResponse\x12\x12\n" + - "\x04sqid\x18\x01 \x01(\tR\x04sqid\";\n" + + "\x04sqid\x18\x01 \x01(\tR\x04sqid\"Q\n" + "\rCancelRequest\x12\x12\n" + "\x04sqid\x18\x01 \x01(\tR\x04sqid\x12\x16\n" + - "\x06reason\x18\x02 \x01(\tR\x06reason\"\x10\n" + + "\x06reason\x18\x02 \x01(\tR\x06reason\x12\x14\n" + + "\x05queue\x18\x03 \x01(\tR\x05queue\"\x10\n" + "\x0eCancelResponse\"\xc9\x02\n" + "\x0eRequestSummary\x12\x12\n" + "\x04sqid\x18\x01 \x01(\tR\x04sqid\x12\x14\n" + @@ -1301,14 +1352,16 @@ const file_gateway_proto_rawDesc = "" + "\bmetadata\x18\a \x03(\v26.uber.submitqueue.gateway.RequestSummary.MetadataEntryR\bmetadata\x1a;\n" + "\rMetadataEntry\x12\x10\n" + "\x03key\x18\x01 \x01(\tR\x03key\x12\x14\n" + - "\x05value\x18\x02 \x01(\tR\x05value:\x028\x01\"2\n" + + "\x05value\x18\x02 \x01(\tR\x05value:\x028\x01\"H\n" + "\x1cGetRequestSummaryByIDRequest\x12\x12\n" + - "\x04sqid\x18\x01 \x01(\tR\x04sqid\"c\n" + + "\x04sqid\x18\x01 \x01(\tR\x04sqid\x12\x14\n" + + "\x05queue\x18\x02 \x01(\tR\x05queue\"c\n" + "\x1dGetRequestSummaryByIDResponse\x12B\n" + - "\arequest\x18\x01 \x01(\v2(.uber.submitqueue.gateway.RequestSummaryR\arequest\"D\n" + + "\arequest\x18\x01 \x01(\v2(.uber.submitqueue.gateway.RequestSummaryR\arequest\"Z\n" + "#GetRequestSummaryByChangeURIRequest\x12\x1d\n" + "\n" + - "change_uri\x18\x01 \x01(\tR\tchangeUri\"l\n" + + "change_uri\x18\x01 \x01(\tR\tchangeUri\x12\x14\n" + + "\x05queue\x18\x02 \x01(\tR\x05queue\"l\n" + "$GetRequestSummaryByChangeURIResponse\x12D\n" + "\brequests\x18\x01 \x03(\v2(.uber.submitqueue.gateway.RequestSummaryR\brequests\"\xc3\x01\n" + "\vListRequest\x12\x14\n" + @@ -1320,9 +1373,10 @@ const file_gateway_proto_rawDesc = "" + "page_token\x18\x05 \x01(\tR\tpageToken\"|\n" + "\fListResponse\x12D\n" + "\brequests\x18\x01 \x03(\v2(.uber.submitqueue.gateway.RequestSummaryR\brequests\x12&\n" + - "\x0fnext_page_token\x18\x02 \x01(\tR\rnextPageToken\"2\n" + + "\x0fnext_page_token\x18\x02 \x01(\tR\rnextPageToken\"H\n" + "\x1cGetRequestHistoryByIDRequest\x12\x12\n" + - "\x04sqid\x18\x01 \x01(\tR\x04sqid\"\xf7\x01\n" + + "\x04sqid\x18\x01 \x01(\tR\x04sqid\x12\x14\n" + + "\x05queue\x18\x02 \x01(\tR\x05queue\"\xf7\x01\n" + "\fHistoryEvent\x12!\n" + "\ftimestamp_ms\x18\x01 \x01(\x03R\vtimestampMs\x12\x16\n" + "\x06status\x18\x02 \x01(\tR\x06status\x12\x1d\n" + @@ -1333,10 +1387,11 @@ const file_gateway_proto_rawDesc = "" + "\x03key\x18\x01 \x01(\tR\x03key\x12\x14\n" + "\x05value\x18\x02 \x01(\tR\x05value:\x028\x01\"_\n" + "\x1dGetRequestHistoryByIDResponse\x12>\n" + - "\x06events\x18\x01 \x03(\v2&.uber.submitqueue.gateway.HistoryEventR\x06events\"D\n" + + "\x06events\x18\x01 \x03(\v2&.uber.submitqueue.gateway.HistoryEventR\x06events\"Z\n" + "#GetRequestHistoryByChangeURIRequest\x12\x1d\n" + "\n" + - "change_uri\x18\x01 \x01(\tR\tchangeUri\"d\n" + + "change_uri\x18\x01 \x01(\tR\tchangeUri\x12\x14\n" + + "\x05queue\x18\x02 \x01(\tR\x05queue\"d\n" + "\x0eRequestHistory\x12\x12\n" + "\x04sqid\x18\x01 \x01(\tR\x04sqid\x12>\n" + "\x06events\x18\x02 \x03(\v2&.uber.submitqueue.gateway.HistoryEventR\x06events\"n\n" + diff --git a/api/submitqueue/gateway/protopb/gateway.pb.yarpc.go b/api/submitqueue/gateway/protopb/gateway.pb.yarpc.go index cb87c2347..68c53c358 100644 --- a/api/submitqueue/gateway/protopb/gateway.pb.yarpc.go +++ b/api/submitqueue/gateway/protopb/gateway.pb.yarpc.go @@ -576,74 +576,75 @@ var ( var yarpcFileDescriptorClosuref1a937782ebbded5 = [][]byte{ // gateway.proto []byte{ - 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0xb4, 0x57, 0x4f, 0x6f, 0x1b, 0x45, - 0x14, 0xcf, 0xda, 0x89, 0x13, 0x3f, 0x3b, 0x21, 0x1a, 0x4a, 0x59, 0x99, 0x44, 0x4d, 0x96, 0x92, - 0xfa, 0x80, 0xec, 0xca, 0x94, 0x3f, 0x22, 0x6a, 0xa5, 0xa6, 0x49, 0x4b, 0x25, 0x52, 0xcc, 0x86, - 0x08, 0x09, 0x09, 0x59, 0x63, 0xfb, 0xd5, 0x59, 0x25, 0xbb, 0xeb, 0xcc, 0xcc, 0x06, 0x1c, 0x71, - 0xe0, 0xc2, 0x85, 0x33, 0x77, 0x3e, 0x0c, 0x27, 0xbe, 0x03, 0xdf, 0x83, 0x2b, 0x9a, 0x3f, 0xbb, - 0xde, 0x4d, 0xbc, 0x1b, 0xa7, 0xc0, 0xc9, 0x3b, 0x6f, 0xde, 0xef, 0xbd, 0x37, 0xbf, 0xf7, 0x67, - 0xc6, 0xb0, 0x3a, 0xa2, 0x02, 0x7f, 0xa0, 0x93, 0xd6, 0x98, 0x85, 0x22, 0x24, 0x76, 0xd4, 0x47, - 0xd6, 0xe2, 0x51, 0xdf, 0xf7, 0xc4, 0x79, 0x84, 0x11, 0xb6, 0xcc, 0x7e, 0xc3, 0xa1, 0x63, 0xaf, - 0xdd, 0xa7, 0x1c, 0xdb, 0x83, 0x13, 0x1a, 0x8c, 0xb0, 0xad, 0x00, 0x66, 0xa1, 0xd1, 0x8d, 0x87, - 0x89, 0x8e, 0x8f, 0x6c, 0x84, 0x5c, 0x30, 0x2a, 0x70, 0x34, 0x31, 0xaa, 0x19, 0x99, 0x46, 0x38, - 0x0f, 0xa0, 0xd6, 0xf5, 0x82, 0x91, 0x8b, 0xe7, 0x11, 0x72, 0x41, 0x6c, 0x58, 0xf6, 0x91, 0x73, - 0x3a, 0x42, 0xdb, 0xda, 0xb2, 0x9a, 0x55, 0x37, 0x5e, 0x3a, 0xbf, 0x58, 0x50, 0xd7, 0x9a, 0x7c, - 0x1c, 0x06, 0x1c, 0xf3, 0x55, 0xc9, 0x36, 0xd4, 0x39, 0xb2, 0x0b, 0x6f, 0x80, 0xbd, 0x80, 0xfa, - 0x68, 0x97, 0xd4, 0x76, 0xcd, 0xc8, 0x5e, 0x51, 0x1f, 0xc9, 0x06, 0x54, 0x85, 0xe7, 0x23, 0x17, - 0xd4, 0x1f, 0xdb, 0xe5, 0x2d, 0xab, 0x59, 0x76, 0xa7, 0x02, 0xd2, 0x80, 0x95, 0x93, 0x90, 0x0b, - 0x05, 0x5e, 0x54, 0xe0, 0x64, 0xed, 0xfc, 0x66, 0x41, 0xed, 0x4b, 0x1a, 0x0c, 0xe3, 0x88, 0xef, - 0xc0, 0x92, 0xe2, 0xc9, 0x04, 0xa1, 0x17, 0xe4, 0x21, 0x54, 0x34, 0x31, 0xca, 0x79, 0xad, 0x63, - 0xb7, 0x14, 0xaf, 0x92, 0x9a, 0x96, 0x61, 0xec, 0x99, 0xfa, 0x71, 0x8d, 0x1e, 0x79, 0x0c, 0x2b, - 0x31, 0x35, 0xca, 0xe7, 0x5a, 0x67, 0x3b, 0x85, 0xc9, 0x52, 0x77, 0x64, 0x3e, 0xdc, 0x04, 0xe2, - 0x38, 0x50, 0xd7, 0x51, 0x19, 0x76, 0x08, 0x2c, 0xf2, 0x73, 0x6f, 0x68, 0xa2, 0x52, 0xdf, 0xce, - 0x2e, 0xac, 0x3e, 0xa3, 0xc1, 0x00, 0xcf, 0xe2, 0xd8, 0x67, 0x28, 0x91, 0xbb, 0x50, 0x61, 0x48, - 0x79, 0x18, 0x18, 0xda, 0xcc, 0xca, 0x59, 0x87, 0xb5, 0x18, 0xac, 0x5d, 0x38, 0x7f, 0x96, 0x60, - 0xcd, 0x58, 0x3a, 0x8a, 0x7c, 0x9f, 0xb2, 0xc9, 0x4c, 0x83, 0x09, 0x41, 0xa5, 0x34, 0x41, 0xf7, - 0xa0, 0xa6, 0x0f, 0xde, 0x8b, 0x98, 0xc7, 0xed, 0xf2, 0x56, 0xb9, 0x59, 0x75, 0x41, 0x8b, 0x8e, - 0x99, 0xc7, 0xc9, 0x7d, 0x58, 0x63, 0x38, 0x40, 0xef, 0x02, 0x87, 0x3d, 0x2a, 0x7a, 0x3e, 0x57, - 0xac, 0x94, 0xdd, 0x7a, 0x2c, 0x7d, 0x2a, 0x0e, 0xb9, 0x8c, 0x96, 0x0b, 0x2a, 0x22, 0x6e, 0x2f, - 0xe9, 0x68, 0xf5, 0x8a, 0x6c, 0x02, 0x9c, 0x51, 0x2e, 0x7a, 0xc8, 0x58, 0xc8, 0xec, 0x8a, 0xda, - 0xab, 0x4a, 0xc9, 0x81, 0x14, 0x10, 0x17, 0x56, 0x7c, 0x14, 0x74, 0x48, 0x05, 0xb5, 0x97, 0xb7, - 0xca, 0xcd, 0x5a, 0xe7, 0x93, 0x56, 0x5e, 0xe1, 0xb7, 0xb2, 0x67, 0x6c, 0x1d, 0x1a, 0xe0, 0x41, - 0x20, 0xd8, 0xc4, 0x4d, 0xec, 0x34, 0x76, 0x61, 0x35, 0xb3, 0x45, 0xd6, 0xa1, 0x7c, 0x8a, 0x13, - 0xc3, 0x85, 0xfc, 0x94, 0x54, 0x5c, 0xd0, 0xb3, 0x29, 0x15, 0x6a, 0xf1, 0x79, 0xe9, 0x33, 0xcb, - 0xe9, 0xc0, 0xc6, 0x0b, 0x14, 0x59, 0x4f, 0x7b, 0x93, 0x97, 0xfb, 0x05, 0x99, 0x72, 0x06, 0xb0, - 0x99, 0x83, 0x31, 0x35, 0xb0, 0x07, 0xcb, 0x4c, 0xef, 0x2a, 0x5c, 0xad, 0xd3, 0x9c, 0xf7, 0x90, - 0x6e, 0x0c, 0x74, 0xf6, 0xe1, 0xfd, 0x19, 0x4e, 0x74, 0xed, 0x1e, 0xbb, 0x2f, 0xe3, 0xf8, 0x36, - 0x01, 0xa6, 0xe9, 0x34, 0x51, 0x56, 0x93, 0x6c, 0x3a, 0x67, 0x70, 0xbf, 0xd8, 0x8a, 0x89, 0x78, - 0x1f, 0x56, 0x8c, 0x63, 0x6e, 0x5b, 0x2a, 0x2f, 0xf3, 0x87, 0x9c, 0x20, 0x9d, 0x3f, 0x64, 0x8b, - 0x7a, 0x5c, 0x14, 0xb7, 0xe8, 0x23, 0x78, 0x37, 0x5d, 0x60, 0x21, 0xeb, 0xd1, 0xd7, 0x02, 0x99, - 0xac, 0xb4, 0x92, 0xaa, 0xb4, 0xb7, 0xa7, 0x95, 0xf6, 0x15, 0x7b, 0x2a, 0xf7, 0x0e, 0x39, 0xf9, - 0x10, 0x48, 0x82, 0xea, 0xe3, 0xeb, 0x90, 0xa1, 0x04, 0xe8, 0x09, 0xb2, 0x1e, 0xef, 0xec, 0xa9, - 0x8d, 0x43, 0x4e, 0xde, 0x83, 0xea, 0x98, 0x8e, 0xb0, 0xc7, 0xbd, 0x4b, 0x3d, 0x49, 0x96, 0xdc, - 0x15, 0x29, 0x38, 0xf2, 0x2e, 0x51, 0x72, 0xa6, 0x36, 0x45, 0x78, 0x8a, 0x81, 0xa9, 0x5f, 0xa5, - 0xfe, 0x8d, 0x14, 0x38, 0x3f, 0x41, 0x5d, 0x1f, 0xe2, 0xbf, 0xe4, 0x86, 0xec, 0xc0, 0x5b, 0x01, - 0xfe, 0x28, 0x7a, 0x29, 0xcf, 0xba, 0x18, 0x57, 0xa5, 0xb8, 0x9b, 0x78, 0xcf, 0x14, 0xe4, 0x17, - 0x1e, 0x17, 0xe1, 0xcd, 0x05, 0xf9, 0xb7, 0x05, 0x75, 0xa3, 0x7a, 0x70, 0x81, 0x81, 0x90, 0x83, - 0x38, 0x19, 0xaa, 0x92, 0x26, 0x4b, 0xd1, 0x54, 0x4b, 0x64, 0x99, 0x06, 0x2e, 0x15, 0x34, 0x70, - 0xf9, 0x6a, 0x03, 0x77, 0x53, 0x0d, 0xbc, 0xa8, 0xc8, 0x78, 0x94, 0x4f, 0x46, 0x3a, 0xa6, 0xff, - 0xa7, 0x7d, 0x7b, 0xe9, 0x56, 0xcc, 0xb0, 0x65, 0x92, 0xf7, 0x04, 0x2a, 0x28, 0xdd, 0xc7, 0xa9, - 0xdb, 0x99, 0x2f, 0x5a, 0xd7, 0xa0, 0xb2, 0x6d, 0x98, 0x38, 0xb8, 0x6d, 0x1b, 0x0e, 0x93, 0x81, - 0x6d, 0x4c, 0xcc, 0x1c, 0xd8, 0xd3, 0x58, 0x4b, 0x6f, 0x14, 0x6b, 0x90, 0x6e, 0xf6, 0x59, 0xb1, - 0x1a, 0x4e, 0x9e, 0x43, 0xf5, 0x44, 0xed, 0x7a, 0x38, 0x7f, 0x45, 0x1b, 0x7b, 0xee, 0x14, 0xea, - 0x6c, 0xc3, 0x92, 0x2e, 0x8a, 0xfc, 0xc7, 0x03, 0xc2, 0xdd, 0xe3, 0x80, 0xe1, 0x20, 0x1c, 0x05, - 0xde, 0x25, 0x0e, 0xbf, 0x96, 0x96, 0x35, 0xe6, 0x63, 0x58, 0xd2, 0x25, 0xa6, 0x27, 0xe4, 0xbd, - 0xfc, 0x00, 0x94, 0xbe, 0xab, 0xb5, 0x67, 0x5f, 0x6a, 0xce, 0xcf, 0x16, 0xdc, 0x31, 0x71, 0xbe, - 0x0a, 0xc5, 0xf3, 0x30, 0x0a, 0x86, 0xff, 0xca, 0x4b, 0x9c, 0x9d, 0x52, 0x2a, 0x3b, 0xd9, 0x14, - 0x97, 0xaf, 0xa4, 0xb8, 0xf3, 0xd7, 0x32, 0x90, 0x23, 0x65, 0x57, 0x1d, 0xf2, 0x85, 0x36, 0x4b, - 0xbe, 0x85, 0x45, 0xf9, 0x78, 0x22, 0x1f, 0xe4, 0x7b, 0x4e, 0x3d, 0xc3, 0x1a, 0x3b, 0x37, 0xa9, - 0x99, 0x27, 0xc0, 0x82, 0x34, 0x2c, 0xdf, 0x1d, 0x45, 0x86, 0x53, 0xaf, 0xa5, 0x22, 0xc3, 0xe9, - 0xe7, 0x8b, 0xb3, 0x40, 0xbe, 0x87, 0x8a, 0x7e, 0x6f, 0x90, 0x07, 0xf9, 0x98, 0xcc, 0x73, 0xa6, - 0xd1, 0xbc, 0x59, 0x31, 0x31, 0xff, 0xab, 0x05, 0xef, 0xcc, 0xbc, 0x3d, 0x49, 0xc1, 0x4b, 0xa0, - 0xe8, 0x8a, 0x6e, 0x7c, 0x7a, 0x6b, 0x5c, 0x12, 0xcc, 0xef, 0xd6, 0xcc, 0xeb, 0x3f, 0x69, 0x19, - 0xf2, 0xf8, 0x56, 0xb6, 0xaf, 0x8e, 0x85, 0xc6, 0x93, 0x37, 0x85, 0x67, 0xd2, 0xec, 0x71, 0x51, - 0x98, 0xe6, 0xe9, 0x8d, 0x5b, 0x98, 0xe6, 0xd4, 0x9d, 0x76, 0x2d, 0x0f, 0xa9, 0xd1, 0x39, 0x5f, - 0x1e, 0xae, 0xdf, 0x4c, 0xf3, 0xe5, 0x61, 0xc6, 0x8c, 0xbe, 0x96, 0x87, 0xeb, 0xa3, 0x6b, 0xbe, - 0x3c, 0xe4, 0x8e, 0xe7, 0xf9, 0xf2, 0x90, 0x3f, 0x31, 0x9d, 0x85, 0xbd, 0x53, 0xd8, 0x18, 0x84, - 0x7e, 0xae, 0x99, 0xbd, 0xba, 0x69, 0xf8, 0xae, 0xfc, 0x73, 0xd5, 0xb5, 0xbe, 0xdb, 0x1d, 0x79, - 0xe2, 0x24, 0xea, 0xb7, 0x06, 0xa1, 0xdf, 0x96, 0xa0, 0x76, 0x0a, 0xd4, 0x96, 0x7f, 0xd6, 0xd2, - 0x6b, 0x63, 0x44, 0xff, 0x5b, 0x1b, 0xf7, 0xfb, 0x15, 0xf5, 0xf1, 0xd1, 0x3f, 0x01, 0x00, 0x00, - 0xff, 0xff, 0x28, 0xf4, 0xb0, 0x04, 0x23, 0x0e, 0x00, 0x00, + 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0xb4, 0x57, 0x4b, 0x6f, 0xdb, 0x46, + 0x10, 0x0e, 0x45, 0x5b, 0xb6, 0x46, 0xb2, 0x6b, 0x6c, 0xd3, 0x94, 0x50, 0x6d, 0xc4, 0x66, 0x53, + 0x47, 0x87, 0x42, 0x0a, 0xdc, 0xf4, 0x81, 0x06, 0x09, 0x10, 0x27, 0xce, 0x03, 0xa8, 0x53, 0x85, + 0xae, 0x51, 0x20, 0x40, 0x21, 0xac, 0xa4, 0x89, 0x4c, 0xd8, 0x24, 0xe5, 0xdd, 0xa5, 0x5b, 0x19, + 0x3d, 0xf4, 0xd2, 0x4b, 0xcf, 0xbd, 0xf7, 0xc7, 0xf4, 0xd4, 0xff, 0xd0, 0xff, 0xd1, 0x6b, 0xb1, + 0x0f, 0x52, 0xa4, 0x2d, 0x32, 0x8a, 0x9b, 0x9e, 0xc4, 0xdd, 0x9d, 0xe7, 0xf7, 0xcd, 0xce, 0x8e, + 0x60, 0x65, 0x44, 0x05, 0xfe, 0x48, 0x27, 0xed, 0x31, 0x8b, 0x44, 0x44, 0x9c, 0xb8, 0x8f, 0xac, + 0xcd, 0xe3, 0x7e, 0xe0, 0x8b, 0xd3, 0x18, 0x63, 0x6c, 0x9b, 0xf3, 0xa6, 0x4b, 0xc7, 0x7e, 0xa7, + 0x4f, 0x39, 0x76, 0x06, 0x47, 0x34, 0x1c, 0x61, 0x47, 0x29, 0x98, 0x85, 0xd6, 0x6e, 0xde, 0x49, + 0x65, 0x02, 0x64, 0x23, 0xe4, 0x82, 0x51, 0x81, 0xa3, 0x89, 0x11, 0xcd, 0xed, 0x69, 0x0d, 0xf7, + 0x36, 0xd4, 0xbb, 0x7e, 0x38, 0xf2, 0xf0, 0x34, 0x46, 0x2e, 0x88, 0x03, 0x4b, 0x01, 0x72, 0x4e, + 0x47, 0xe8, 0x58, 0x9b, 0x56, 0xab, 0xe6, 0x25, 0x4b, 0xf7, 0x57, 0x0b, 0x1a, 0x5a, 0x92, 0x8f, + 0xa3, 0x90, 0x63, 0xb1, 0x28, 0xd9, 0x82, 0x06, 0x47, 0x76, 0xe6, 0x0f, 0xb0, 0x17, 0xd2, 0x00, + 0x9d, 0x8a, 0x3a, 0xae, 0x9b, 0xbd, 0x17, 0x34, 0x40, 0xb2, 0x0e, 0x35, 0xe1, 0x07, 0xc8, 0x05, + 0x0d, 0xc6, 0x8e, 0xbd, 0x69, 0xb5, 0x6c, 0x6f, 0xba, 0x41, 0x9a, 0xb0, 0x7c, 0x14, 0x71, 0xa1, + 0x94, 0x17, 0x94, 0x72, 0xba, 0x76, 0x7f, 0xb7, 0xa0, 0xfe, 0x0d, 0x0d, 0x87, 0x49, 0xc4, 0xd7, + 0x61, 0x51, 0xe1, 0x64, 0x82, 0xd0, 0x0b, 0x72, 0x07, 0xaa, 0x1a, 0x18, 0xe5, 0xbc, 0xbe, 0xe3, + 0xb4, 0x15, 0xae, 0x12, 0x9a, 0xb6, 0x41, 0xec, 0x91, 0xfa, 0xf1, 0x8c, 0x1c, 0xb9, 0x0f, 0xcb, + 0x09, 0x34, 0xca, 0xe7, 0xea, 0xce, 0x56, 0x46, 0x27, 0x0f, 0xdd, 0x81, 0xf9, 0xf0, 0x52, 0x15, + 0xd7, 0x85, 0x86, 0x8e, 0xca, 0xa0, 0x43, 0x60, 0x81, 0x9f, 0xfa, 0x43, 0x13, 0x95, 0xfa, 0x76, + 0x5f, 0xc2, 0xca, 0x23, 0x1a, 0x0e, 0xf0, 0x24, 0x89, 0x7d, 0x86, 0x10, 0xb9, 0x01, 0x55, 0x86, + 0x94, 0x47, 0xa1, 0x81, 0xcd, 0xac, 0xa6, 0x79, 0xda, 0x99, 0x3c, 0xdd, 0x35, 0x58, 0x4d, 0x4c, + 0x6a, 0xc7, 0xee, 0x5f, 0x15, 0x58, 0x35, 0xf6, 0x0f, 0xe2, 0x20, 0xa0, 0x6c, 0x32, 0xd3, 0x4d, + 0x6a, 0xae, 0x92, 0x85, 0xed, 0x26, 0xd4, 0x35, 0x1c, 0xbd, 0x98, 0xf9, 0xdc, 0xb1, 0x37, 0xed, + 0x56, 0xcd, 0x03, 0xbd, 0x75, 0xc8, 0x7c, 0x4e, 0x6e, 0xc1, 0x2a, 0xc3, 0x01, 0xfa, 0x67, 0x38, + 0xec, 0x51, 0xd1, 0x0b, 0xb8, 0xc2, 0xca, 0xf6, 0x1a, 0xc9, 0xee, 0x43, 0xb1, 0xcf, 0x65, 0x0e, + 0x5c, 0x50, 0x11, 0x73, 0x67, 0x51, 0xe7, 0xa0, 0x57, 0x64, 0x03, 0xe0, 0x84, 0x72, 0xd1, 0x43, + 0xc6, 0x22, 0xe6, 0x54, 0xd5, 0x59, 0x4d, 0xee, 0xec, 0xc9, 0x0d, 0xe2, 0xc1, 0x72, 0x80, 0x82, + 0x0e, 0xa9, 0xa0, 0xce, 0xd2, 0xa6, 0xdd, 0xaa, 0xef, 0x7c, 0xd1, 0x2e, 0xba, 0x0e, 0xed, 0x7c, + 0x8e, 0xed, 0x7d, 0xa3, 0xb8, 0x17, 0x0a, 0x36, 0xf1, 0x52, 0x3b, 0xcd, 0x7b, 0xb0, 0x92, 0x3b, + 0x22, 0x6b, 0x60, 0x1f, 0xe3, 0xc4, 0x60, 0x21, 0x3f, 0x25, 0x14, 0x67, 0xf4, 0x64, 0x0a, 0x85, + 0x5a, 0x7c, 0x5d, 0xf9, 0xca, 0x72, 0x9f, 0xc1, 0xfa, 0x53, 0x14, 0x79, 0x4f, 0xbb, 0x93, 0xe7, + 0x8f, 0xcb, 0xf8, 0x9b, 0x09, 0xac, 0x3b, 0x80, 0x8d, 0x02, 0x4b, 0xa6, 0x5e, 0x76, 0x61, 0x89, + 0xe9, 0x53, 0x65, 0xad, 0xbe, 0xd3, 0x9a, 0x37, 0x75, 0x2f, 0x51, 0x74, 0x5f, 0xc1, 0xc7, 0x33, + 0x9c, 0xe8, 0x3a, 0x3f, 0xf4, 0x9e, 0x27, 0x51, 0x6f, 0x00, 0x4c, 0x49, 0x36, 0xb1, 0xd7, 0x52, + 0x8e, 0x0b, 0x12, 0x38, 0x81, 0x5b, 0xe5, 0xb6, 0x4d, 0x1e, 0x8f, 0x61, 0xd9, 0x84, 0xc3, 0x1d, + 0x4b, 0x71, 0x38, 0x7f, 0x22, 0xa9, 0xa6, 0xfb, 0xa7, 0xbc, 0xe4, 0x3e, 0x17, 0xe5, 0x97, 0xfc, + 0x2e, 0x7c, 0x98, 0x2d, 0xc6, 0x88, 0xf5, 0xe8, 0x6b, 0x81, 0x4c, 0x56, 0x65, 0x45, 0x55, 0xe5, + 0xfb, 0xd3, 0xaa, 0xfc, 0x96, 0x3d, 0x94, 0x67, 0xfb, 0x9c, 0x7c, 0x0a, 0x24, 0xd5, 0xea, 0xe3, + 0xeb, 0x88, 0xa1, 0x54, 0xd0, 0x3d, 0x68, 0x2d, 0x39, 0xd9, 0x55, 0x07, 0xfb, 0x9c, 0x7c, 0x04, + 0xb5, 0x31, 0x1d, 0x61, 0x8f, 0xfb, 0xe7, 0xba, 0x17, 0x2d, 0x7a, 0xcb, 0x72, 0xe3, 0xc0, 0x3f, + 0x47, 0x89, 0xa4, 0x3a, 0x14, 0xd1, 0x31, 0x86, 0xa6, 0xd6, 0x95, 0xf8, 0x77, 0x72, 0xc3, 0xfd, + 0x19, 0x1a, 0x3a, 0x89, 0x77, 0x89, 0x0d, 0xd9, 0x86, 0xf7, 0x42, 0xfc, 0x49, 0xf4, 0x32, 0x9e, + 0x35, 0x53, 0x2b, 0x72, 0xbb, 0x9b, 0x7a, 0xcf, 0x15, 0xef, 0x33, 0x9f, 0x8b, 0xe8, 0xaa, 0xc5, + 0xfb, 0x8f, 0x05, 0x0d, 0x63, 0x60, 0xef, 0x0c, 0x43, 0x21, 0x1b, 0x7c, 0xda, 0xac, 0x25, 0x78, + 0x96, 0x02, 0xaf, 0x9e, 0xee, 0xe5, 0x5a, 0x40, 0xa5, 0xa4, 0x05, 0xd8, 0x17, 0x5b, 0x40, 0x37, + 0xd3, 0x02, 0x16, 0x14, 0x44, 0x77, 0x8b, 0x21, 0xca, 0xc6, 0xf4, 0xff, 0x34, 0x80, 0x5e, 0xf6, + 0xda, 0xe6, 0x30, 0x34, 0x94, 0x3e, 0x80, 0x2a, 0x4a, 0xf7, 0x09, 0xa1, 0xdb, 0xf3, 0x45, 0xeb, + 0x19, 0xad, 0xfc, 0x95, 0x4d, 0x1d, 0xbc, 0x9b, 0x2b, 0x3b, 0x4c, 0x1f, 0x02, 0x63, 0x78, 0x26, + 0xe5, 0xd3, 0x0c, 0x2a, 0x57, 0xca, 0x20, 0xcc, 0x36, 0x86, 0x59, 0x19, 0x18, 0xa4, 0x9e, 0x40, + 0xed, 0x48, 0x9d, 0xfa, 0x38, 0x7f, 0xf5, 0x1b, 0x7b, 0xde, 0x54, 0xd5, 0xdd, 0x82, 0x45, 0x5d, + 0x2a, 0xc5, 0xa3, 0x0a, 0xc2, 0x8d, 0xc3, 0x90, 0xe1, 0x20, 0x1a, 0x85, 0xfe, 0x39, 0x0e, 0x5f, + 0x4a, 0xcb, 0x5a, 0xe7, 0x73, 0x58, 0xd4, 0x85, 0xa7, 0x7b, 0xec, 0xcd, 0xe2, 0x00, 0x94, 0xbc, + 0xa7, 0xa5, 0x0b, 0xf0, 0xfd, 0xc5, 0x82, 0xeb, 0x26, 0xce, 0x17, 0x91, 0x78, 0x12, 0xc5, 0xe1, + 0xf0, 0x3f, 0x79, 0x49, 0xd8, 0xa9, 0x64, 0xd8, 0xc9, 0x13, 0x6f, 0x5f, 0x20, 0x7e, 0xe7, 0xef, + 0x25, 0x20, 0x07, 0xca, 0xae, 0x4a, 0xf2, 0xa9, 0x36, 0x4b, 0xbe, 0x87, 0x05, 0x39, 0xaa, 0x91, + 0x4f, 0x8a, 0x3d, 0x67, 0x86, 0xbe, 0xe6, 0xf6, 0x9b, 0xc4, 0xcc, 0x68, 0x71, 0x4d, 0x1a, 0x96, + 0x53, 0x4e, 0x99, 0xe1, 0xcc, 0x6c, 0x56, 0x66, 0x38, 0x3b, 0x2c, 0xb9, 0xd7, 0xc8, 0x0f, 0x50, + 0xd5, 0x73, 0x0c, 0xb9, 0x5d, 0xac, 0x93, 0x1b, 0x9e, 0x9a, 0xad, 0x37, 0x0b, 0xa6, 0xe6, 0x7f, + 0xb3, 0xe0, 0x83, 0x99, 0xef, 0x2f, 0x29, 0x99, 0x30, 0xca, 0x9e, 0xfe, 0xe6, 0x97, 0x6f, 0xad, + 0x97, 0x06, 0xf3, 0x87, 0x35, 0x73, 0xac, 0x48, 0xaf, 0x0c, 0xb9, 0xff, 0x56, 0xb6, 0x2f, 0x36, + 0x8b, 0xe6, 0x83, 0xab, 0xaa, 0xe7, 0x68, 0xf6, 0xb9, 0x28, 0xa5, 0x79, 0xfa, 0x3a, 0x97, 0xd2, + 0x9c, 0x79, 0xff, 0x2e, 0xf1, 0x90, 0x69, 0xa8, 0xf3, 0xf1, 0x70, 0xf9, 0x15, 0x9b, 0x8f, 0x87, + 0x19, 0x9d, 0xfb, 0x12, 0x0f, 0x97, 0x5b, 0xd7, 0x7c, 0x3c, 0x14, 0x36, 0xed, 0xf9, 0x78, 0x28, + 0xee, 0x98, 0xee, 0xb5, 0xdd, 0x63, 0x58, 0x1f, 0x44, 0x41, 0xa1, 0x99, 0xdd, 0x86, 0xb9, 0xf0, + 0x5d, 0xf9, 0x57, 0xae, 0x6b, 0xbd, 0xba, 0x37, 0xf2, 0xc5, 0x51, 0xdc, 0x6f, 0x0f, 0xa2, 0xa0, + 0x23, 0x95, 0x3a, 0x19, 0xa5, 0x8e, 0xfc, 0x6b, 0x98, 0x5d, 0x1b, 0x23, 0xfa, 0xbf, 0xe1, 0xb8, + 0xdf, 0xaf, 0xaa, 0x8f, 0xcf, 0xfe, 0x0d, 0x00, 0x00, 0xff, 0xff, 0x54, 0xbb, 0x2f, 0xff, 0x91, + 0x0e, 0x00, 0x00, }, // api/base/change/proto/change.proto []byte{ diff --git a/doc/rfc/submitqueue/history-api.md b/doc/rfc/submitqueue/history-api.md index f8899075f..b9155fb6c 100644 --- a/doc/rfc/submitqueue/history-api.md +++ b/doc/rfc/submitqueue/history-api.md @@ -18,6 +18,8 @@ The gateway exposes two read-only RPCs because an `sqid` selects one event list message GetRequestHistoryByIDRequest { // Globally unique identifier for a request, as returned by Land. string sqid = 1; + // Queue processing the request. Required: a sqid is only resolvable within its own queue. + string queue = 2; } message HistoryEvent { @@ -39,6 +41,8 @@ message GetRequestHistoryByIDResponse { message GetRequestHistoryByChangeURIRequest { // Exact change URI supplied to Land. string change_uri = 1; + // Queue to search. Required: results are scoped to one queue. + string queue = 2; } message RequestHistory { @@ -118,6 +122,7 @@ Error behavior follows the conventions established by request-summary retrieval: - An empty `sqid` passed to `GetRequestHistoryByID` is an invalid request. - An empty `change_uri` passed to `GetRequestHistoryByChangeURI` is an invalid request. +- An empty `queue` passed to either RPC is an invalid request. Both selectors are queue-scoped so that the retained history is shardable by queue; a sqid or change URI from another queue is reported as not found. - If no request-log records exist for an `sqid`, `GetRequestHistoryByID` returns the existing `RequestNotFoundError`. - If no retained request histories match a `change_uri`, `GetRequestHistoryByChangeURI` returns a change-URI-specific not-found user error. - A request-log storage failure is returned as an infrastructure error. @@ -127,10 +132,13 @@ Using the existing request not-found error for `sqid` lookups keeps point lookup ## Flow ```text -GetRequestHistoryByIDRequest(sqid) +GetRequestHistoryByIDRequest(queue, sqid) | v -validate sqid +validate queue and sqid + | + v +resolve the queue's stores | v RequestLogStore.List(sqid) @@ -141,13 +149,16 @@ project each RequestLog to one HistoryEvent v GetRequestHistoryByIDResponse(events) -GetRequestHistoryByChangeURIRequest(change_uri) +GetRequestHistoryByChangeURIRequest(queue, change_uri) + | + v +validate queue and change_uri | v -validate change_uri +resolve the queue's stores | v -resolve matching sqids +resolve matching sqids within the queue | v RequestLogStore.List(sqid) for each match diff --git a/doc/rfc/submitqueue/status-list-api.md b/doc/rfc/submitqueue/status-list-api.md index 720e3e330..36d82c408 100644 --- a/doc/rfc/submitqueue/status-list-api.md +++ b/doc/rfc/submitqueue/status-list-api.md @@ -22,7 +22,7 @@ The append-only request log is not shaped for the second or third query. Serving 1. The gateway exposes `GetRequestSummaryByID` for sqid lookup and `GetRequestSummaryByChangeURI` for exact change URI lookup. 2. A change URI lookup returns all requests containing that exact URI, ordered by receipt time descending. -3. Change URIs are treated as globally meaningful identifiers. `GetRequestSummaryByChangeURI` does not require a queue. +3. Every read selector is queue-scoped. `GetRequestSummaryByID` and `GetRequestSummaryByChangeURI` both require a queue alongside the sqid or change URI, so every table is shardable by queue. A change URI landed into several queues matches separately in each, and looking it up across queues is one call per queue. 4. `List` accepts one queue and a required receipt-time range. It does not accept sqid or URI selectors. 5. The `List` time range is based only on gateway receipt time, not lifecycle overlap. 6. The request-summary RPCs and `List` return the same materialized current state and immutable request context. Neither endpoint returns the request-log timeline. @@ -49,7 +49,7 @@ rpc GetRequestSummaryByID(GetRequestSummaryByIDRequest) returns (GetRequestSumma rpc GetRequestSummaryByChangeURI(GetRequestSummaryByChangeURIRequest) returns (GetRequestSummaryByChangeURIResponse) {} ``` -`GetRequestSummaryByID` requires a non-empty sqid. `GetRequestSummaryByChangeURI` requires a non-empty exact change URI. +`GetRequestSummaryByID` requires a non-empty sqid and a non-empty queue. `GetRequestSummaryByChangeURI` requires a non-empty exact change URI and a non-empty queue. A sqid is only resolvable within its own queue, so naming a queue the request does not belong to is reported as not found rather than as a mismatch. An sqid lookup returns exactly one request summary when found. A change URI lookup returns every matching request summary ordered by `(received_at_ms DESC, sqid DESC)`. The sqid tie-breaker makes the result deterministic when requests share a millisecond. @@ -81,11 +81,11 @@ The gateway owns the append-only request log and three new logical read models. ### Request Summary by Sqid -The authoritative request summary is keyed by sqid. It contains immutable request context plus the current materialized request-log winner and the reconciliation state needed to compare a later log entry without rereading historical logs. +The authoritative request summary is keyed by `(queue, sqid)`. It contains immutable request context plus the current materialized request-log winner and the reconciliation state needed to compare a later log entry without rereading historical logs. The immutable context is queue, change URIs, and receipt time. The mutable response state is status, last error, and metadata. The internal `accepting` admission state is not returned by the API. The projection version fields used for optimistic conditional writes are internal and are not part of the API response. -The sqid key supports authoritative lookup and conditional status updates for one request without a secondary index. +The key supports authoritative lookup and conditional status updates for one request without a secondary index, and leads with the queue so the table is shardable by queue. ### Request Summaries by Queue @@ -97,11 +97,11 @@ The logical key covers the `List` queue predicate, receipt-time range, newest-fi ### Requests by Change URI -The URI reverse mapping is logically keyed by `(change_uri, received_at_ms, sqid)` and must support a bounded descending scan over `(received_at_ms, sqid)`. As with the queue projection, a backend may use a reverse range scan or descending-encoded key components while exposing cursors and results in the original values. The mapping contains immutable lookup data and does not duplicate mutable status fields. +The URI reverse mapping is logically keyed by `(queue, change_uri, received_at_ms, sqid)` and must support a bounded descending scan over `(received_at_ms, sqid)` within one queue. As with the queue projection, a backend may use a reverse range scan or descending-encoded key components while exposing cursors and results in the original values. The mapping contains immutable lookup data and does not duplicate mutable status fields. The mapping repeats `received_at_ms` because receipt time is part of the promised newest-first ordering. This allows the gateway to perform a bounded ordered scan before resolving the matching authoritative summaries. Without receipt time in the mapping, the gateway would have to fetch and sort every request associated with a URI before enforcing the result maximum. -The logical key supports the bounded `GetRequestSummaryByChangeURI(change_uri)` newest-first scan and deterministic sqid tie-breaker without fetching every matching summary first. +The logical key supports the bounded `GetRequestSummaryByChangeURI(queue, change_uri)` newest-first scan and deterministic sqid tie-breaker without fetching every matching summary first, and leads with the queue so the table is shardable by queue. The URI is stored in the canonical form received from the validated Land request. URI normalization rules belong to the change contract or source-control integration and are not introduced by this read model. diff --git a/platform/extension/counter/README.md b/platform/extension/counter/README.md index 29240b5d7..8e2d48979 100644 --- a/platform/extension/counter/README.md +++ b/platform/extension/counter/README.md @@ -1,35 +1,38 @@ # Counter -Vendor-agnostic interface for atomic sequential number generation. +Vendor-agnostic interface for atomic sequential number generation, scoped per queue. ## Interface -### Counter +### Factory -Generates unique, sequential values scoped to a domain string. +Resolves the Counter bound to one queue. The host wiring decides which backend serves which queue; a resolved instance can only advance that queue's sequences. -```go -type Counter interface { - Next(ctx context.Context, domain string) (int64, error) -} -``` +### Counter -- **domain**: A string key that scopes the counter (max 255 characters). Each domain maintains its own independent sequence. +Generates unique, sequential values scoped to a domain string within the bound queue. + +- **domain**: A string key naming a sequence within the queue (max 255 characters). Each `(queue, domain)` pair maintains its own independent sequence. - **Next**: Atomically increments and returns the next value. The first call for a new domain returns 1. Safe for concurrent use; values are unique but ordering is not guaranteed. +The domain is a sequence *name*, not a queue-qualified key — callers pass `"request"` or `"batch"`, never `"request/my-queue"`. Callers that embed the queue in a minted identifier build that string themselves, independently of the domain, so the two cannot drift into each other. + ## Usage ```go -cnt := mysqlcounter.NewCounter(db) +cnt, err := factory.For(counter.Config{QueueName: "my-queue"}) + +val, err := cnt.Next(ctx, "request") // returns 1 +val, err = cnt.Next(ctx, "request") // returns 2 +val, err = cnt.Next(ctx, "batch") // returns 1, an independent sequence -// Generate sequential IDs for different domains -val, err := cnt.Next(ctx, "request/my-queue") // returns 1 -val, err = cnt.Next(ctx, "request/my-queue") // returns 2 -val, err = cnt.Next(ctx, "request/other") // returns 1 +other, err := factory.For(counter.Config{QueueName: "other-queue"}) +val, err = other.Next(ctx, "request") // returns 1, isolated from my-queue ``` ## Implementing a Backend 1. Create `platform/extension/counter/{backend}/` directory -2. Implement the `Counter` interface -3. Add a schema file under `platform/extension/counter/{backend}/schema/` if the backend requires it +2. Implement the `Counter` interface, binding the queue at construction +3. Add a schema file under `platform/extension/counter/{backend}/schema/` if the backend requires it. The queue must lead the primary key so the table is shardable by queue. +4. Adapt the constructor to the `Factory` interface in the wiring layer, not here diff --git a/platform/extension/counter/counter.go b/platform/extension/counter/counter.go index b419a25e0..e272d2878 100644 --- a/platform/extension/counter/counter.go +++ b/platform/extension/counter/counter.go @@ -18,9 +18,29 @@ package counter import "context" -// Counter provides atomic sequential number generation for a given domain. +// Config identifies the queue a Counter instance is resolved for. Like every +// other extension config, it carries only the queue name — everything an +// implementation needs beyond that is injected at construction by the +// integrator. +type Config struct { + // QueueName is the name of the queue whose sequences the resolved Counter + // is scoped to. + QueueName string +} + +// Factory resolves the queue-scoped Counter for a queue. Mirrors the extension +// contract: the host wiring decides which backend serves which queue; +// implementations bind the queue over their backend so a resolved instance can +// only read and advance that queue's sequences. +type Factory interface { + // For returns the Counter bound to the queue named in config. + For(config Config) (Counter, error) +} + +// Counter provides atomic sequential number generation for a given domain +// within the queue the instance is bound to. // Each call to Next returns the next value in the sequence for the specified domain. -// The value is guaranteed to be unique within the domain throughout the system and persisted accordingly. +// The value is guaranteed to be unique within the (queue, domain) pair throughout the system and persisted accordingly. type Counter interface { // Next atomically increments the counter for the given domain and returns the new value. // The first call for a new domain returns 1. diff --git a/platform/extension/counter/mock/BUILD.bazel b/platform/extension/counter/mock/BUILD.bazel index aad76476c..994b2e265 100644 --- a/platform/extension/counter/mock/BUILD.bazel +++ b/platform/extension/counter/mock/BUILD.bazel @@ -5,5 +5,8 @@ go_library( srcs = ["counter_mock.go"], importpath = "github.com/uber/submitqueue/platform/extension/counter/mock", visibility = ["//visibility:public"], - deps = ["@org_uber_go_mock//gomock:go_default_library"], + deps = [ + "//platform/extension/counter:go_default_library", + "@org_uber_go_mock//gomock:go_default_library", + ], ) diff --git a/platform/extension/counter/mock/counter_mock.go b/platform/extension/counter/mock/counter_mock.go index 5f1d18af8..55be58ae4 100644 --- a/platform/extension/counter/mock/counter_mock.go +++ b/platform/extension/counter/mock/counter_mock.go @@ -13,9 +13,49 @@ import ( context "context" reflect "reflect" + counter "github.com/uber/submitqueue/platform/extension/counter" gomock "go.uber.org/mock/gomock" ) +// MockFactory is a mock of Factory interface. +type MockFactory struct { + ctrl *gomock.Controller + recorder *MockFactoryMockRecorder + isgomock struct{} +} + +// MockFactoryMockRecorder is the mock recorder for MockFactory. +type MockFactoryMockRecorder struct { + mock *MockFactory +} + +// NewMockFactory creates a new mock instance. +func NewMockFactory(ctrl *gomock.Controller) *MockFactory { + mock := &MockFactory{ctrl: ctrl} + mock.recorder = &MockFactoryMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockFactory) EXPECT() *MockFactoryMockRecorder { + return m.recorder +} + +// For mocks base method. +func (m *MockFactory) For(config counter.Config) (counter.Counter, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "For", config) + ret0, _ := ret[0].(counter.Counter) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// For indicates an expected call of For. +func (mr *MockFactoryMockRecorder) For(config any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "For", reflect.TypeOf((*MockFactory)(nil).For), config) +} + // MockCounter is a mock of Counter interface. type MockCounter struct { ctrl *gomock.Controller diff --git a/platform/extension/counter/mysql/counter.go b/platform/extension/counter/mysql/counter.go index db9ab9658..ec34e3c7b 100644 --- a/platform/extension/counter/mysql/counter.go +++ b/platform/extension/counter/mysql/counter.go @@ -27,29 +27,33 @@ import ( type mysqlCounter struct { db *sql.DB scope tally.Scope + // queue is the queue name this counter instance is bound to; every sequence + // it advances is scoped to it. + queue string } -// NewCounter creates a new MySQL-backed Counter. -func NewCounter(db *sql.DB, scope tally.Scope) counter.Counter { - return &mysqlCounter{db: db, scope: scope} +// NewCounter creates a new MySQL-backed Counter bound to queue. +func NewCounter(db *sql.DB, scope tally.Scope, queue string) counter.Counter { + return &mysqlCounter{db: db, scope: scope, queue: queue} } -// Next atomically increments the counter for the given domain and returns the new value. +// Next atomically increments the counter for the given domain within the bound queue +// and returns the new value. // Uses MySQL's LAST_INSERT_ID() to set the value atomically and read the incremented value. func (c *mysqlCounter) Next(ctx context.Context, domain string) (ret int64, retErr error) { op := metrics.Begin(c.scope, "next", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() result, err := c.db.ExecContext(ctx, - "INSERT INTO counter (domain, value) VALUES (?, LAST_INSERT_ID(1)) ON DUPLICATE KEY UPDATE value = LAST_INSERT_ID(value + 1)", - domain, + "INSERT INTO counter (queue, domain, value) VALUES (?, ?, LAST_INSERT_ID(1)) ON DUPLICATE KEY UPDATE value = LAST_INSERT_ID(value + 1)", + c.queue, domain, ) if err != nil { - return 0, fmt.Errorf("failed to increment counter for domain=%s: %w", domain, err) + return 0, fmt.Errorf("failed to increment counter for queue=%s domain=%s: %w", c.queue, domain, err) } value, err := result.LastInsertId() if err != nil { - return 0, fmt.Errorf("failed to get counter value for domain=%s: %w", domain, err) + return 0, fmt.Errorf("failed to get counter value for queue=%s domain=%s: %w", c.queue, domain, err) } return value, nil diff --git a/platform/extension/counter/mysql/schema/counter.sql b/platform/extension/counter/mysql/schema/counter.sql index 754d4d797..7174455b4 100644 --- a/platform/extension/counter/mysql/schema/counter.sql +++ b/platform/extension/counter/mysql/schema/counter.sql @@ -1,5 +1,8 @@ +-- counter holds one monotonic sequence per (queue, domain). queue leads the PK so the +-- table is shardable by queue; domain names the sequence within it (e.g. "request", "batch"). CREATE TABLE IF NOT EXISTS counter ( + queue VARCHAR(255) NOT NULL, domain VARCHAR(255) NOT NULL, value BIGINT NOT NULL, - PRIMARY KEY (domain) + PRIMARY KEY (queue, domain) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; diff --git a/service/stovepipe/server/BUILD.bazel b/service/stovepipe/server/BUILD.bazel index 7ca451418..c76638292 100644 --- a/service/stovepipe/server/BUILD.bazel +++ b/service/stovepipe/server/BUILD.bazel @@ -12,6 +12,7 @@ go_library( "//platform/errs/generic:go_default_library", "//platform/errs/mysql:go_default_library", "//platform/extension/consumergate/noop:go_default_library", + "//platform/extension/counter:go_default_library", "//platform/extension/messagequeue:go_default_library", "//platform/extension/messagequeue/mysql:go_default_library", "//service/stovepipe/server/mapper:go_default_library", diff --git a/service/stovepipe/server/main.go b/service/stovepipe/server/main.go index 6485ccba4..8841a8bea 100644 --- a/service/stovepipe/server/main.go +++ b/service/stovepipe/server/main.go @@ -34,6 +34,7 @@ import ( genericerrs "github.com/uber/submitqueue/platform/errs/generic" mysqlerrs "github.com/uber/submitqueue/platform/errs/mysql" consumergatenoop "github.com/uber/submitqueue/platform/extension/consumergate/noop" + "github.com/uber/submitqueue/platform/extension/counter" extqueue "github.com/uber/submitqueue/platform/extension/messagequeue" queueMySQL "github.com/uber/submitqueue/platform/extension/messagequeue/mysql" "github.com/uber/submitqueue/service/stovepipe/server/mapper" @@ -84,18 +85,46 @@ func (s *StovepipeServer) Ingest(ctx context.Context, req *pb.IngestRequest) (*p type inMemoryCounter struct { mu sync.Mutex values map[string]int64 + queue string } -func newInMemoryCounter() *inMemoryCounter { - return &inMemoryCounter{values: make(map[string]int64)} +func newInMemoryCounter(queue string) *inMemoryCounter { + return &inMemoryCounter{values: make(map[string]int64), queue: queue} } // Next returns the next value in the sequence for the given domain, starting at 1. func (c *inMemoryCounter) Next(_ context.Context, domain string) (int64, error) { c.mu.Lock() defer c.mu.Unlock() - c.values[domain]++ - return c.values[domain], nil + key := c.queue + "\x00" + domain + c.values[key]++ + return c.values[key], nil +} + +// inMemoryCounterFactory is the example counter.Factory: one process-local sequence +// set per queue, minted on first use. A real deployment supplies a persistent factory. +type inMemoryCounterFactory struct { + mu sync.Mutex + counters map[string]*inMemoryCounter +} + +func newInMemoryCounterFactory() *inMemoryCounterFactory { + return &inMemoryCounterFactory{counters: make(map[string]*inMemoryCounter)} +} + +// For returns the process-local Counter bound to the queue named in config. +func (f *inMemoryCounterFactory) For(config counter.Config) (counter.Counter, error) { + if config.QueueName == "" { + return nil, fmt.Errorf("queue name must not be empty") + } + f.mu.Lock() + defer f.mu.Unlock() + if existing, ok := f.counters[config.QueueName]; ok { + return existing, nil + } + created := newInMemoryCounter(config.QueueName) + f.counters[config.QueueName] = created + return created, nil } // fakeSourceControlFactory is the example SourceControl factory. It seeds each queue with a @@ -282,7 +311,7 @@ func run() error { ingestController := controller.NewIngestController( logger.Sugar(), scope, - newInMemoryCounter(), + newInMemoryCounterFactory(), scf, storageFty, registry, diff --git a/service/submitqueue/gateway/server/BUILD.bazel b/service/submitqueue/gateway/server/BUILD.bazel index 71e23a700..410ac58ea 100644 --- a/service/submitqueue/gateway/server/BUILD.bazel +++ b/service/submitqueue/gateway/server/BUILD.bazel @@ -19,6 +19,7 @@ go_library( "//platform/extension/consumergate:go_default_library", "//platform/extension/consumergate/file:go_default_library", "//platform/extension/consumergate/noop:go_default_library", + "//platform/extension/counter:go_default_library", "//platform/extension/counter/mysql:go_default_library", "//platform/extension/messagequeue:go_default_library", "//platform/extension/messagequeue/mysql:go_default_library", diff --git a/service/submitqueue/gateway/server/main.go b/service/submitqueue/gateway/server/main.go index f60bdaca6..a5556f048 100644 --- a/service/submitqueue/gateway/server/main.go +++ b/service/submitqueue/gateway/server/main.go @@ -36,6 +36,7 @@ import ( "github.com/uber/submitqueue/platform/extension/consumergate" consumergatefile "github.com/uber/submitqueue/platform/extension/consumergate/file" consumergatenoop "github.com/uber/submitqueue/platform/extension/consumergate/noop" + "github.com/uber/submitqueue/platform/extension/counter" mysqlcounter "github.com/uber/submitqueue/platform/extension/counter/mysql" extqueue "github.com/uber/submitqueue/platform/extension/messagequeue" queueMySQL "github.com/uber/submitqueue/platform/extension/messagequeue/mysql" @@ -226,8 +227,8 @@ func run() error { } defer appDB.Close() - // Initialize counter from shared app database connection - cnt := mysqlcounter.NewCounter(appDB, scope.SubScope("counter")) + // Initialize counter factory from shared app database connection + cnt := counterFactory{db: appDB, scope: scope.SubScope("counter")} // Open queue database connection queueDSN := os.Getenv("QUEUE_MYSQL_DSN") @@ -323,27 +324,16 @@ func run() error { return fmt.Errorf("failed to load queue configs: %w", err) } - // Create controllers and wrap them for gRPC. The global read-model stores - // are injected individually; queue-scoped storage resolves through the - // factory adapter, and land/cancel/log share one materializer. + // Create controllers and wrap them for gRPC. Every store is queue-scoped and + // resolves through the factory adapter; land/cancel/log share one materializer. storageFty := storageFactory{backend: store} - materializer := requestcore.NewMaterializer(store.GetRequestLogStore(), store.GetRequestSummaryStore(), store.GetRequestURIStore(), storageFty) + materializer := requestcore.NewMaterializer(storageFty) pingController := controller.NewPingController(logger, scope) - landController := controller.NewLandController(logger.Sugar(), scope, cnt, store.GetRequestSummaryStore(), materializer, queueConfigs, registry) - cancelController := controller.NewCancelController(logger.Sugar(), scope, store.GetRequestSummaryStore(), materializer, registry) - requestSummaryController := controller.NewRequestSummaryController( - logger.Sugar(), - scope, - store.GetRequestSummaryStore(), - store.GetRequestURIStore(), - ) + landController := controller.NewLandController(logger.Sugar(), scope, cnt, storageFty, materializer, queueConfigs, registry) + cancelController := controller.NewCancelController(logger.Sugar(), scope, storageFty, materializer, registry) + requestSummaryController := controller.NewRequestSummaryController(logger.Sugar(), scope, storageFty) listController := controller.NewListController(logger.Sugar(), scope, storageFty, queueConfigs) - requestHistoryController := controller.NewRequestHistoryController( - logger.Sugar(), - scope, - store.GetRequestLogStore(), - store.GetRequestURIStore(), - ) + requestHistoryController := controller.NewRequestHistoryController(logger.Sugar(), scope, storageFty) gatewayServer := &GatewayServer{ pingController: pingController, landController: landController, @@ -476,3 +466,20 @@ type storageFactory struct { func (f storageFactory) For(config storage.Config) (storage.Storage, error) { return f.backend.For(config.QueueName) } + +// counterFactory adapts the MySQL counter backend to the counter.Factory seam. +// Routing every queue to the single shared database is this host's policy; a +// deployment that splits queues across backends swaps this adapter for a +// routing one. +type counterFactory struct { + db *sql.DB + scope tally.Scope +} + +// For returns the Counter bound to the queue named in config. +func (f counterFactory) For(config counter.Config) (counter.Counter, error) { + if config.QueueName == "" { + return nil, fmt.Errorf("queue name must not be empty") + } + return mysqlcounter.NewCounter(f.db, f.scope, config.QueueName), nil +} diff --git a/service/submitqueue/gateway/server/mapper/cancel.go b/service/submitqueue/gateway/server/mapper/cancel.go index 6a505e82a..157ffa25a 100644 --- a/service/submitqueue/gateway/server/mapper/cancel.go +++ b/service/submitqueue/gateway/server/mapper/cancel.go @@ -24,6 +24,7 @@ import ( func ProtoToCancelRequest(req *pb.CancelRequest) entity.CancelRequest { return entity.CancelRequest{ ID: req.GetSqid(), + Queue: req.GetQueue(), Reason: req.GetReason(), } } diff --git a/service/submitqueue/gateway/server/mapper/cancel_test.go b/service/submitqueue/gateway/server/mapper/cancel_test.go index ccdd14da3..cc9525663 100644 --- a/service/submitqueue/gateway/server/mapper/cancel_test.go +++ b/service/submitqueue/gateway/server/mapper/cancel_test.go @@ -29,14 +29,14 @@ func TestProtoToCancelRequest(t *testing.T) { expected entity.CancelRequest }{ { - name: "maps sqid and reason", - req: &pb.CancelRequest{Sqid: "test-queue/42", Reason: "obsolete change"}, - expected: entity.CancelRequest{ID: "test-queue/42", Reason: "obsolete change"}, + name: "maps sqid, queue and reason", + req: &pb.CancelRequest{Sqid: "test-queue/42", Queue: "test-queue", Reason: "obsolete change"}, + expected: entity.CancelRequest{ID: "test-queue/42", Queue: "test-queue", Reason: "obsolete change"}, }, { - name: "maps sqid without reason", - req: &pb.CancelRequest{Sqid: "test-queue/1"}, - expected: entity.CancelRequest{ID: "test-queue/1"}, + name: "maps sqid and queue without reason", + req: &pb.CancelRequest{Sqid: "test-queue/1", Queue: "test-queue"}, + expected: entity.CancelRequest{ID: "test-queue/1", Queue: "test-queue"}, }, { name: "empty request yields zero value", diff --git a/service/submitqueue/gateway/server/mapper/request_history.go b/service/submitqueue/gateway/server/mapper/request_history.go index c50864b3d..89ff14232 100644 --- a/service/submitqueue/gateway/server/mapper/request_history.go +++ b/service/submitqueue/gateway/server/mapper/request_history.go @@ -21,12 +21,12 @@ import ( // ProtoToGetRequestHistoryByIDRequest maps the wire request to the entity request the controller operates on. func ProtoToGetRequestHistoryByIDRequest(req *pb.GetRequestHistoryByIDRequest) entity.GetRequestHistoryByIDRequest { - return entity.GetRequestHistoryByIDRequest{ID: req.GetSqid()} + return entity.GetRequestHistoryByIDRequest{ID: req.GetSqid(), Queue: req.GetQueue()} } // ProtoToGetRequestHistoryByChangeURIRequest maps the wire request to the entity request the controller operates on. func ProtoToGetRequestHistoryByChangeURIRequest(req *pb.GetRequestHistoryByChangeURIRequest) entity.GetRequestHistoryByChangeURIRequest { - return entity.GetRequestHistoryByChangeURIRequest{ChangeURI: req.GetChangeUri()} + return entity.GetRequestHistoryByChangeURIRequest{ChangeURI: req.GetChangeUri(), Queue: req.GetQueue()} } // HistoryEventsToProto maps retained request-log events to wire history events. diff --git a/service/submitqueue/gateway/server/mapper/request_history_test.go b/service/submitqueue/gateway/server/mapper/request_history_test.go index 79b565d5d..53c14e257 100644 --- a/service/submitqueue/gateway/server/mapper/request_history_test.go +++ b/service/submitqueue/gateway/server/mapper/request_history_test.go @@ -24,12 +24,12 @@ import ( func TestProtoToGetRequestHistoryRequests(t *testing.T) { assert.Equal(t, - entity.GetRequestHistoryByIDRequest{ID: "q/1"}, - ProtoToGetRequestHistoryByIDRequest(&pb.GetRequestHistoryByIDRequest{Sqid: "q/1"}), + entity.GetRequestHistoryByIDRequest{ID: "q/1", Queue: "q"}, + ProtoToGetRequestHistoryByIDRequest(&pb.GetRequestHistoryByIDRequest{Sqid: "q/1", Queue: "q"}), ) assert.Equal(t, - entity.GetRequestHistoryByChangeURIRequest{ChangeURI: "uri"}, - ProtoToGetRequestHistoryByChangeURIRequest(&pb.GetRequestHistoryByChangeURIRequest{ChangeUri: "uri"}), + entity.GetRequestHistoryByChangeURIRequest{ChangeURI: "uri", Queue: "q"}, + ProtoToGetRequestHistoryByChangeURIRequest(&pb.GetRequestHistoryByChangeURIRequest{ChangeUri: "uri", Queue: "q"}), ) } diff --git a/service/submitqueue/gateway/server/mapper/request_summary.go b/service/submitqueue/gateway/server/mapper/request_summary.go index 55204aded..bb7f6eda7 100644 --- a/service/submitqueue/gateway/server/mapper/request_summary.go +++ b/service/submitqueue/gateway/server/mapper/request_summary.go @@ -22,7 +22,8 @@ import ( // ProtoToGetRequestSummaryByIDRequest maps the wire request to the entity request the controller operates on. func ProtoToGetRequestSummaryByIDRequest(req *pb.GetRequestSummaryByIDRequest) entity.GetRequestSummaryByIDRequest { return entity.GetRequestSummaryByIDRequest{ - ID: req.GetSqid(), + ID: req.GetSqid(), + Queue: req.GetQueue(), } } @@ -30,6 +31,7 @@ func ProtoToGetRequestSummaryByIDRequest(req *pb.GetRequestSummaryByIDRequest) e func ProtoToGetRequestSummaryByChangeURIRequest(req *pb.GetRequestSummaryByChangeURIRequest) entity.GetRequestSummaryByChangeURIRequest { return entity.GetRequestSummaryByChangeURIRequest{ ChangeURI: req.GetChangeUri(), + Queue: req.GetQueue(), } } diff --git a/service/submitqueue/gateway/server/mapper/request_summary_test.go b/service/submitqueue/gateway/server/mapper/request_summary_test.go index ccb600c2c..275df3757 100644 --- a/service/submitqueue/gateway/server/mapper/request_summary_test.go +++ b/service/submitqueue/gateway/server/mapper/request_summary_test.go @@ -24,15 +24,15 @@ import ( func TestProtoToGetRequestSummaryByIDRequest(t *testing.T) { assert.Equal(t, - entity.GetRequestSummaryByIDRequest{ID: "test-queue/42"}, - ProtoToGetRequestSummaryByIDRequest(&pb.GetRequestSummaryByIDRequest{Sqid: "test-queue/42"}), + entity.GetRequestSummaryByIDRequest{ID: "test-queue/42", Queue: "test-queue"}, + ProtoToGetRequestSummaryByIDRequest(&pb.GetRequestSummaryByIDRequest{Sqid: "test-queue/42", Queue: "test-queue"}), ) } func TestProtoToGetRequestSummaryByChangeURIRequest(t *testing.T) { assert.Equal(t, - entity.GetRequestSummaryByChangeURIRequest{ChangeURI: "github://uber/repo/pull/1/abc"}, - ProtoToGetRequestSummaryByChangeURIRequest(&pb.GetRequestSummaryByChangeURIRequest{ChangeUri: "github://uber/repo/pull/1/abc"}), + entity.GetRequestSummaryByChangeURIRequest{ChangeURI: "github://uber/repo/pull/1/abc", Queue: "test-queue"}, + ProtoToGetRequestSummaryByChangeURIRequest(&pb.GetRequestSummaryByChangeURIRequest{ChangeUri: "github://uber/repo/pull/1/abc", Queue: "test-queue"}), ) } diff --git a/service/submitqueue/orchestrator/server/BUILD.bazel b/service/submitqueue/orchestrator/server/BUILD.bazel index 3e7ad9378..af45a02fb 100644 --- a/service/submitqueue/orchestrator/server/BUILD.bazel +++ b/service/submitqueue/orchestrator/server/BUILD.bazel @@ -20,6 +20,7 @@ go_library( "//platform/extension/consumergate:go_default_library", "//platform/extension/consumergate/file:go_default_library", "//platform/extension/consumergate/noop:go_default_library", + "//platform/extension/counter:go_default_library", "//platform/extension/counter/mysql:go_default_library", "//platform/extension/messagequeue/mysql:go_default_library", "//platform/http:go_default_library", diff --git a/service/submitqueue/orchestrator/server/main.go b/service/submitqueue/orchestrator/server/main.go index 2fc2d8e07..8f752805c 100644 --- a/service/submitqueue/orchestrator/server/main.go +++ b/service/submitqueue/orchestrator/server/main.go @@ -37,6 +37,7 @@ import ( "github.com/uber/submitqueue/platform/extension/consumergate" consumergatefile "github.com/uber/submitqueue/platform/extension/consumergate/file" consumergatenoop "github.com/uber/submitqueue/platform/extension/consumergate/noop" + "github.com/uber/submitqueue/platform/extension/counter" mysqlcounter "github.com/uber/submitqueue/platform/extension/counter/mysql" queueMySQL "github.com/uber/submitqueue/platform/extension/messagequeue/mysql" "github.com/uber/submitqueue/platform/http" @@ -140,7 +141,7 @@ func run() error { } defer appDB.Close() - cnt := mysqlcounter.NewCounter(appDB, scope.SubScope("counter")) + cnt := counterFactory{db: appDB, scope: scope.SubScope("counter")} store, err := mysqlstorage.NewStorage(appDB, scope.SubScope("storage")) if err != nil { @@ -446,3 +447,20 @@ type storageFactory struct { func (f storageFactory) For(config storage.Config) (storage.Storage, error) { return f.backend.For(config.QueueName) } + +// counterFactory adapts the MySQL counter backend to the counter.Factory seam. +// Routing every queue to the single shared database is this host's policy; a +// deployment that splits queues across backends swaps this adapter for a +// routing one. +type counterFactory struct { + db *sql.DB + scope tally.Scope +} + +// For returns the Counter bound to the queue named in config. +func (f counterFactory) For(config counter.Config) (counter.Counter, error) { + if config.QueueName == "" { + return nil, fmt.Errorf("queue name must not be empty") + } + return mysqlcounter.NewCounter(f.db, f.scope, config.QueueName), nil +} diff --git a/stovepipe/controller/BUILD.bazel b/stovepipe/controller/BUILD.bazel index da72c78ae..5f683870e 100644 --- a/stovepipe/controller/BUILD.bazel +++ b/stovepipe/controller/BUILD.bazel @@ -34,6 +34,7 @@ go_test( deps = [ "//api/stovepipe/protopb:go_default_library", "//platform/consumer:go_default_library", + "//platform/extension/counter:go_default_library", "//platform/extension/counter/mock:go_default_library", "//platform/extension/messagequeue/mock:go_default_library", "//stovepipe/core/messagequeue:go_default_library", diff --git a/stovepipe/controller/ingest.go b/stovepipe/controller/ingest.go index a9253b0f9..b8a0b649d 100644 --- a/stovepipe/controller/ingest.go +++ b/stovepipe/controller/ingest.go @@ -36,6 +36,11 @@ import ( // This error should be mapped to codes.InvalidArgument at the gRPC layer. var ErrInvalidRequest = errs.NewUserError(errors.New("invalid request")) +// counterDomainRequest names the per-queue sequence that mints request IDs. It also +// happens to be the leading segment of the ID, but the two are written independently +// (see resolveID) so they cannot drift into each other. +const counterDomainRequest = "request" + // IsInvalidRequest returns true if any error in the error chain is ErrInvalidRequest. func IsInvalidRequest(err error) bool { return errors.Is(err, ErrInvalidRequest) @@ -51,7 +56,7 @@ func IsInvalidRequest(err error) bool { type IngestController struct { logger *zap.SugaredLogger metricsScope tally.Scope - counter counter.Counter + counters counter.Factory sourceControl sourcecontrol.Factory stores storage.Factory registry consumer.TopicRegistry @@ -62,7 +67,7 @@ type IngestController struct { func NewIngestController( logger *zap.SugaredLogger, scope tally.Scope, - counter counter.Counter, + counters counter.Factory, sourceControl sourcecontrol.Factory, stores storage.Factory, registry consumer.TopicRegistry, @@ -70,7 +75,7 @@ func NewIngestController( return &IngestController{ logger: logger, metricsScope: scope.SubScope("ingest_controller"), - counter: counter, + counters: counters, sourceControl: sourceControl, stores: stores, registry: registry, @@ -167,14 +172,19 @@ func (c *IngestController) resolveID(ctx context.Context, store storage.Storage, return "", fmt.Errorf("failed to look up existing request for queue=%s: %w", queue, err) } - // Mint a globally unique request ID namespaced by the queue. The counter domain - // ("request/") doubles as the ID prefix, so the ID is "/". - domain := "request/" + queue - seq, err := c.counter.Next(ctx, domain) + // Mint a globally unique request ID namespaced by the queue. The ID format + // ("request//") is written out here rather than derived from the counter + // domain: the domain is a per-queue sequence name only, and the two must stay independent + // so re-keying the counter cannot change the emitted ID. + queueCounter, err := c.counters.For(counter.Config{QueueName: queue}) + if err != nil { + return "", fmt.Errorf("failed to resolve counter for queue=%s: %w", queue, err) + } + seq, err := queueCounter.Next(ctx, counterDomainRequest) if err != nil { return "", fmt.Errorf("failed to generate request ID for queue=%s: %w", queue, err) } - id := fmt.Sprintf("%s/%d", domain, seq) + id := fmt.Sprintf("%s/%s/%d", counterDomainRequest, queue, seq) if err := uriStore.Create(ctx, uri, id); err != nil { if errors.Is(err, storage.ErrAlreadyExists) { diff --git a/stovepipe/controller/ingest_test.go b/stovepipe/controller/ingest_test.go index af8cec379..0ead32a70 100644 --- a/stovepipe/controller/ingest_test.go +++ b/stovepipe/controller/ingest_test.go @@ -23,6 +23,7 @@ import ( "github.com/stretchr/testify/require" "github.com/uber-go/tally" "github.com/uber/submitqueue/platform/consumer" + "github.com/uber/submitqueue/platform/extension/counter" countermock "github.com/uber/submitqueue/platform/extension/counter/mock" mqmock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" @@ -57,6 +58,12 @@ type staticStorageFactory struct{ store storage.Storage } // For returns the fixed store aggregate for any queue. func (f staticStorageFactory) For(storage.Config) (storage.Storage, error) { return f.store, nil } +// staticCounterFactory resolves every queue to the same counter, so tests can keep +// setting expectations on one mock regardless of which queue the controller resolves. +type staticCounterFactory struct{ counter counter.Counter } + +func (f staticCounterFactory) For(counter.Config) (counter.Counter, error) { return f.counter, nil } + func newIngestController(t *testing.T, ctrl *gomock.Controller) (*IngestController, ingestMocks) { t.Helper() @@ -83,7 +90,7 @@ func newIngestController(t *testing.T, ctrl *gomock.Controller) (*IngestControll }) require.NoError(t, err) - c := NewIngestController(zap.NewNop().Sugar(), tally.NewTestScope("test", nil), m.counter, m.factory, staticStorageFactory{store: store}, registry) + c := NewIngestController(zap.NewNop().Sugar(), tally.NewTestScope("test", nil), staticCounterFactory{counter: m.counter}, m.factory, staticStorageFactory{store: store}, registry) return c, m } @@ -125,7 +132,7 @@ func TestIngestController_Ingest(t *testing.T) { setup: func(m ingestMocks) { expectResolve(m) m.uriStore.EXPECT().GetIDByURI(gomock.Any(), testURI).Return("", storage.ErrNotFound) - m.counter.EXPECT().Next(gomock.Any(), "request/"+testQueue).Return(int64(7), nil) + m.counter.EXPECT().Next(gomock.Any(), counterDomainRequest).Return(int64(7), nil) m.uriStore.EXPECT().Create(gomock.Any(), testURI, "request/monorepo/main/7").Return(nil) m.reqStore.EXPECT().Get(gomock.Any(), "request/monorepo/main/7").Return(entity.Request{}, storage.ErrNotFound) m.reqStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) @@ -165,7 +172,7 @@ func TestIngestController_Ingest(t *testing.T) { setup: func(m ingestMocks) { expectResolve(m) m.uriStore.EXPECT().GetIDByURI(gomock.Any(), testURI).Return("", storage.ErrNotFound) - m.counter.EXPECT().Next(gomock.Any(), "request/"+testQueue).Return(int64(7), nil) + m.counter.EXPECT().Next(gomock.Any(), counterDomainRequest).Return(int64(7), nil) m.uriStore.EXPECT().Create(gomock.Any(), testURI, "request/monorepo/main/7").Return(storage.ErrAlreadyExists) m.uriStore.EXPECT().GetIDByURI(gomock.Any(), testURI).Return("request/monorepo/main/3", nil) m.reqStore.EXPECT().Get(gomock.Any(), "request/monorepo/main/3").Return(entity.Request{ID: "request/monorepo/main/3", State: entity.RequestStateAccepted}, nil) diff --git a/submitqueue/core/request/log.go b/submitqueue/core/request/log.go index c85cb9049..eef6615e2 100644 --- a/submitqueue/core/request/log.go +++ b/submitqueue/core/request/log.go @@ -60,9 +60,10 @@ func PublishLog(ctx context.Context, registry consumer.TopicRegistry, logEntry e // PublishBatchLogs publishes a request log entry for each request ID in the batch to the log topic. // Each entry uses the request ID as the partition key to ensure per-request ordering. -func PublishBatchLogs(ctx context.Context, registry consumer.TopicRegistry, requestIDs []string, status entity.RequestStatus, metadata map[string]string) error { +// queue scopes every entry: a request ID is only unique within its own queue. +func PublishBatchLogs(ctx context.Context, registry consumer.TopicRegistry, queue string, requestIDs []string, status entity.RequestStatus, metadata map[string]string) error { for _, requestID := range requestIDs { - logEntry := entity.NewRequestLog(requestID, status, 0, "", metadata) + logEntry := entity.NewRequestLog(queue, requestID, status, 0, "", metadata) if err := PublishLog(ctx, registry, logEntry, requestID); err != nil { return fmt.Errorf("failed to publish request log for request %s: %w", requestID, err) } diff --git a/submitqueue/core/request/log_test.go b/submitqueue/core/request/log_test.go index 39dd65c98..60c911ff8 100644 --- a/submitqueue/core/request/log_test.go +++ b/submitqueue/core/request/log_test.go @@ -50,7 +50,7 @@ func TestPublishLog_Success(t *testing.T) { ctrl := gomock.NewController(t) registry := newTestRegistry(t, ctrl, nil) - logEntry := entity.NewRequestLog("req/1", entity.RequestStatusStarted, 1, "", nil) + logEntry := entity.NewRequestLog("req", "req/1", entity.RequestStatusStarted, 1, "", nil) err := PublishLog(context.Background(), registry, logEntry, "req/1") require.NoError(t, err) } @@ -59,7 +59,7 @@ func TestPublishLog_PublishFailure(t *testing.T) { ctrl := gomock.NewController(t) registry := newTestRegistry(t, ctrl, fmt.Errorf("connection refused")) - logEntry := entity.NewRequestLog("req/1", entity.RequestStatusStarted, 1, "", nil) + logEntry := entity.NewRequestLog("req", "req/1", entity.RequestStatusStarted, 1, "", nil) err := PublishLog(context.Background(), registry, logEntry, "req/1") require.Error(t, err) } @@ -68,8 +68,7 @@ func TestPublishBatchLogs_Success(t *testing.T) { ctrl := gomock.NewController(t) registry := newTestRegistry(t, ctrl, nil) - err := PublishBatchLogs(context.Background(), registry, - []string{"req/1", "req/2", "req/3"}, + err := PublishBatchLogs(context.Background(), registry, "req", []string{"req/1", "req/2", "req/3"}, entity.RequestStatusBatched, map[string]string{"batch_id": "b/1"}, ) @@ -99,8 +98,7 @@ func TestPublishBatchLogs_PartialFailure(t *testing.T) { ) require.NoError(t, err) - err = PublishBatchLogs(context.Background(), registry, - []string{"req/1", "req/2", "req/3"}, + err = PublishBatchLogs(context.Background(), registry, "req", []string{"req/1", "req/2", "req/3"}, entity.RequestStatusBatched, map[string]string{"batch_id": "b/1"}, ) @@ -111,7 +109,7 @@ func TestPublishBatchLogs_Empty(t *testing.T) { ctrl := gomock.NewController(t) registry := newTestRegistry(t, ctrl, nil) - err := PublishBatchLogs(context.Background(), registry, nil, entity.RequestStatusBatched, nil) + err := PublishBatchLogs(context.Background(), registry, "req", nil, entity.RequestStatusBatched, nil) require.NoError(t, err) } @@ -149,11 +147,11 @@ func TestPublishLog_MessageIDScopedByStatus(t *testing.T) { entity.RequestStatusCancelled, } { require.NoError(t, PublishLog(context.Background(), registry, - entity.NewRequestLog("req/1", st, 0, "", nil), "req/1")) + entity.NewRequestLog("req", "req/1", st, 0, "", nil), "req/1")) } // Re-emit "started" to simulate a retry of the same delivery — must reuse the same ID. require.NoError(t, PublishLog(context.Background(), registry, - entity.NewRequestLog("req/1", entity.RequestStatusStarted, 0, "", nil), "req/1")) + entity.NewRequestLog("req", "req/1", entity.RequestStatusStarted, 0, "", nil), "req/1")) require.Equal(t, []string{ "req/1/started", diff --git a/submitqueue/core/request/materializer.go b/submitqueue/core/request/materializer.go index 7cf247d3c..ec1318ed6 100644 --- a/submitqueue/core/request/materializer.go +++ b/submitqueue/core/request/materializer.go @@ -27,31 +27,34 @@ import ( // Materializer appends request logs and projects the winning public request state. // It owns winner selection, optimistic concurrency, and public projection repair. -// The global read-model stores are injected individually; the queue-scoped -// summary projection is resolved per record through the factory, using the -// queue carried on the authoritative summary. +// Every store it touches is queue-scoped, so each call resolves the aggregate once +// from the queue carried on the log being persisted. type Materializer struct { - logs storage.RequestLogStore - summaries storage.RequestSummaryStore - uris storage.RequestURIStore - stores storage.Factory + stores storage.Factory } // NewMaterializer creates a request read-model materializer. -func NewMaterializer(logs storage.RequestLogStore, summaries storage.RequestSummaryStore, uris storage.RequestURIStore, stores storage.Factory) *Materializer { - return &Materializer{logs: logs, summaries: summaries, uris: uris, stores: stores} +func NewMaterializer(stores storage.Factory) *Materializer { + return &Materializer{stores: stores} } // PersistLog appends one audit log and materializes its winning state. // Projection errors are returned so queue deliveries are retried rather than silently dropping the side write. // Because the append happens first, retrying after a projection failure may retain another copy of the event in History. func (m *Materializer) PersistLog(ctx context.Context, log entity.RequestLog) error { - if err := m.logs.Insert(ctx, log); err != nil { + stores, err := m.stores.For(storage.Config{QueueName: log.Queue}) + if err != nil { + return fmt.Errorf("failed to resolve storage for queue %q: %w", log.Queue, err) + } + logs := stores.GetRequestLogStore() + summaries := stores.GetRequestSummaryStore() + + if err := logs.Insert(ctx, log); err != nil { return fmt.Errorf("failed to insert request log request_id=%s: %w", log.RequestID, err) } for { - summary, err := m.summaries.Get(ctx, log.RequestID) + summary, err := summaries.Get(ctx, log.RequestID) if err != nil { return fmt.Errorf("failed to get request summary request_id=%s: %w", log.RequestID, err) } @@ -66,7 +69,7 @@ func (m *Materializer) PersistLog(ctx context.Context, log entity.RequestLog) er updated.LastError = log.LastError updated.Metadata = cloneMetadata(log.Metadata) - if err := m.summaries.Update(ctx, updated, oldVersion, newVersion); err != nil { + if err := summaries.Update(ctx, updated, oldVersion, newVersion); err != nil { if errors.Is(err, storage.ErrVersionMismatch) { continue } @@ -76,7 +79,7 @@ func (m *Materializer) PersistLog(ctx context.Context, log entity.RequestLog) er summary = updated } - if err := m.repairPublicProjections(ctx, summary); err != nil { + if err := m.repairPublicProjections(ctx, stores, summary); err != nil { return err } return nil @@ -85,17 +88,13 @@ func (m *Materializer) PersistLog(ctx context.Context, log entity.RequestLog) er // repairPublicProjections activates and repairs the public query projections. // URI mappings are created before the queue summary, which acts as the marker that activation completed. -func (m *Materializer) repairPublicProjections(ctx context.Context, authoritative entity.RequestSummary) error { +func (m *Materializer) repairPublicProjections(ctx context.Context, stores storage.Storage, authoritative entity.RequestSummary) error { desired := queueSummaryFromSummary(authoritative) - queueStores, err := m.stores.For(storage.Config{QueueName: desired.Queue}) - if err != nil { - return fmt.Errorf("failed to resolve storage for queue %q: %w", desired.Queue, err) - } - queueSummaries := queueStores.GetRequestQueueSummaryStore() + queueSummaries := stores.GetRequestQueueSummaryStore() for { current, err := queueSummaries.Get(ctx, desired.ReceivedAtMs, desired.RequestID) if errors.Is(err, storage.ErrNotFound) { - if err := m.createURIMappings(ctx, authoritative); err != nil { + if err := m.createURIMappings(ctx, stores, authoritative); err != nil { return err } if err := queueSummaries.Create(ctx, desired); err != nil { @@ -126,14 +125,16 @@ func (m *Materializer) repairPublicProjections(ctx context.Context, authoritativ } } -func (m *Materializer) createURIMappings(ctx context.Context, summary entity.RequestSummary) error { +func (m *Materializer) createURIMappings(ctx context.Context, stores storage.Storage, summary entity.RequestSummary) error { + uris := stores.GetRequestURIStore() for _, changeURI := range summary.ChangeURIs { mapping := entity.RequestURI{ ChangeURI: changeURI, + Queue: summary.Queue, ReceivedAtMs: summary.ReceivedAtMs, RequestID: summary.RequestID, } - if err := m.uris.Create(ctx, mapping); err != nil && !errors.Is(err, storage.ErrAlreadyExists) { + if err := uris.Create(ctx, mapping); err != nil && !errors.Is(err, storage.ErrAlreadyExists) { return fmt.Errorf("failed to create request URI mapping request_id=%s change_uri=%s: %w", summary.RequestID, changeURI, err) } } diff --git a/submitqueue/core/request/materializer_test.go b/submitqueue/core/request/materializer_test.go index a8be57ec2..787a3963d 100644 --- a/submitqueue/core/request/materializer_test.go +++ b/submitqueue/core/request/materializer_test.go @@ -107,8 +107,8 @@ func TestMaterializer_PersistLog(t *testing.T) { activated.StatusTimestampMs = 20 activated.Version = 2 queueStore.EXPECT().Get(gomock.Any(), int64(10), "q/1").Return(entity.RequestQueueSummary{}, storage.ErrNotFound) - uriStore.EXPECT().Create(gomock.Any(), entity.RequestURI{ChangeURI: "uri/1", ReceivedAtMs: 10, RequestID: "q/1"}).Return(nil) - uriStore.EXPECT().Create(gomock.Any(), entity.RequestURI{ChangeURI: "uri/2", ReceivedAtMs: 10, RequestID: "q/1"}).Return(nil) + uriStore.EXPECT().Create(gomock.Any(), entity.RequestURI{ChangeURI: "uri/1", Queue: "q", ReceivedAtMs: 10, RequestID: "q/1"}).Return(nil) + uriStore.EXPECT().Create(gomock.Any(), entity.RequestURI{ChangeURI: "uri/2", Queue: "q", ReceivedAtMs: 10, RequestID: "q/1"}).Return(nil) queueStore.EXPECT().Create(gomock.Any(), queueSummaryFromSummary(activated)).Return(nil) require.NoError(t, m.PersistLog(context.Background(), log)) }) @@ -256,9 +256,12 @@ func materializerStores(ctrl *gomock.Controller) (*Materializer, *storagemock.Mo logStore := storagemock.NewMockRequestLogStore(ctrl) queueScoped := storagemock.NewMockStorage(ctrl) queueScoped.EXPECT().GetRequestQueueSummaryStore().Return(queueStore).AnyTimes() + queueScoped.EXPECT().GetRequestSummaryStore().Return(summaryStore).AnyTimes() + queueScoped.EXPECT().GetRequestURIStore().Return(uriStore).AnyTimes() + queueScoped.EXPECT().GetRequestLogStore().Return(logStore).AnyTimes() factory := storagemock.NewMockFactory(ctrl) factory.EXPECT().For(gomock.Any()).Return(queueScoped, nil).AnyTimes() - return NewMaterializer(logStore, summaryStore, uriStore, factory), summaryStore, queueStore, uriStore, logStore + return NewMaterializer(factory), summaryStore, queueStore, uriStore, logStore } func testRequestSummary() entity.RequestSummary { diff --git a/submitqueue/core/request/terminate.go b/submitqueue/core/request/terminate.go index b48419e17..c46634905 100644 --- a/submitqueue/core/request/terminate.go +++ b/submitqueue/core/request/terminate.go @@ -140,7 +140,7 @@ func TerminateRequest( logVersion = request.Version } - logEntry := entity.NewRequestLog(requestID, status, logVersion, lastError, metadata) + logEntry := entity.NewRequestLog(request.Queue, requestID, status, logVersion, lastError, metadata) if err := PublishLog(ctx, registry, logEntry, requestID); err != nil { return TerminationResult{}, fmt.Errorf("failed to publish request log for %s: %w", requestID, err) } diff --git a/submitqueue/entity/request_history.go b/submitqueue/entity/request_history.go index 9ca585fb3..9c450a9a5 100644 --- a/submitqueue/entity/request_history.go +++ b/submitqueue/entity/request_history.go @@ -18,12 +18,18 @@ package entity type GetRequestHistoryByIDRequest struct { // ID is the globally unique identifier of the request. ID string + // Queue is the name of the queue processing the request. It scopes the lookup: + // a request is only resolvable within its own queue. + Queue string } // GetRequestHistoryByChangeURIRequest identifies retained histories by an exact pinned change URI. type GetRequestHistoryByChangeURIRequest struct { // ChangeURI is the exact change URI supplied in a Land request. ChangeURI string + // Queue is the name of the queue to search. It scopes the lookup: a change URI + // landed into several queues matches separately in each. + Queue string } // RequestHistory groups retained events for one request. diff --git a/submitqueue/entity/request_log.go b/submitqueue/entity/request_log.go index 4d99b27d2..343c049d7 100644 --- a/submitqueue/entity/request_log.go +++ b/submitqueue/entity/request_log.go @@ -97,6 +97,9 @@ const ( type RequestLog struct { // RequestID is the ID of the request this log entry belongs to. References entity.Request.ID. RequestID string `json:"request_id"` + // Queue is the name of the queue processing the request. It is unique together + // with RequestID: a request ID is only unique within its own queue. + Queue string `json:"queue"` // TimestampMs is the time this log entry was created, in milliseconds since Unix epoch. TimestampMs int64 `json:"timestamp_ms"` // Status is the request status at the time this log entry was created. It may contain requests states from the state machine and also display-friendly intermediate statuses. @@ -114,15 +117,17 @@ type RequestLog struct { // NewRequestLog creates a new RequestLog with the given fields. // TimestampMs is set to the current time. If metadata is nil, it will be initialized as an empty map. +// queue is the queue processing the request; it scopes requestID, which is only unique within it. // requestVersion is the version of the request entity, should only be set if reporting a request state as a status, otherwise it should be 0. // lastError is the last error message associated with the status at the time of this log entry, empty string if no error. // metadata is a set of key-value pairs providing additional context for this log entry. Not constrained to any specific format or schema, used for display or debugging purposes. -func NewRequestLog(requestID string, status RequestStatus, requestVersion int32, lastError string, metadata map[string]string) RequestLog { +func NewRequestLog(queue string, requestID string, status RequestStatus, requestVersion int32, lastError string, metadata map[string]string) RequestLog { if metadata == nil { metadata = make(map[string]string) } return RequestLog{ RequestID: requestID, + Queue: queue, TimestampMs: time.Now().UnixMilli(), Status: status, RequestVersion: requestVersion, diff --git a/submitqueue/entity/request_log_test.go b/submitqueue/entity/request_log_test.go index 602bf7e2a..43f89bbd9 100644 --- a/submitqueue/entity/request_log_test.go +++ b/submitqueue/entity/request_log_test.go @@ -22,7 +22,7 @@ import ( ) func TestNewRequestLog_NilMetadata(t *testing.T) { - log := NewRequestLog("queue1/100", RequestStatusStarted, 0, "", nil) + log := NewRequestLog("queue1", "queue1/100", RequestStatusStarted, 0, "", nil) assert.NotNil(t, log.Metadata) assert.Empty(t, log.Metadata) diff --git a/submitqueue/entity/request_summary.go b/submitqueue/entity/request_summary.go index e8a75009b..7d098620d 100644 --- a/submitqueue/entity/request_summary.go +++ b/submitqueue/entity/request_summary.go @@ -18,12 +18,18 @@ package entity type GetRequestSummaryByIDRequest struct { // ID is the globally unique identifier of the request. Format: "/". ID string + // Queue is the name of the queue processing the request. It scopes the lookup: + // a request is only resolvable within its own queue. + Queue string } // GetRequestSummaryByChangeURIRequest identifies request summaries by an exact pinned change URI. type GetRequestSummaryByChangeURIRequest struct { // ChangeURI is the exact change URI supplied in a Land request. ChangeURI string + // Queue is the name of the queue to search. It scopes the lookup: a change URI + // landed into several queues matches separately in each. + Queue string } // RequestSummary is the gateway-owned materialized current view of a request. @@ -75,6 +81,9 @@ type RequestQueueSummary struct { type RequestURI struct { // ChangeURI is the exact canonical URI supplied at receipt. ChangeURI string + // Queue is the name of the queue the request was received into. It scopes the + // mapping: the same change URI landed into several queues maps separately in each. + Queue string // ReceivedAtMs is the immutable receipt timestamp in Unix milliseconds. ReceivedAtMs int64 // RequestID is the globally unique request identifier. diff --git a/submitqueue/entity/speculation.go b/submitqueue/entity/speculation.go index 1794d5b85..fad0f2133 100644 --- a/submitqueue/entity/speculation.go +++ b/submitqueue/entity/speculation.go @@ -155,8 +155,11 @@ type SpeculationPathEntry struct { // Every path in the set shares the same head and assumptions over the same // ordered dependency list. type SpeculationPathSet struct { - // Head is the primary key: the ID of the head batch these paths speculate - // on. Every path in the set carries this same head. + // Queue is the name of the queue the head batch belongs to. It is unique + // together with Head. + Queue string + // Head is the ID of the head batch these paths speculate on, unique within + // Queue. Every path in the set carries this same head. Head string // Paths is the head's chosen paths, live and recently finished. Paths []SpeculationPathEntry diff --git a/submitqueue/extension/storage/mock/storage_mock.go b/submitqueue/extension/storage/mock/storage_mock.go index 5f6ed73f1..d7cedc1c8 100644 --- a/submitqueue/extension/storage/mock/storage_mock.go +++ b/submitqueue/extension/storage/mock/storage_mock.go @@ -163,6 +163,20 @@ func (mr *MockStorageMockRecorder) GetRequestBatchStore() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRequestBatchStore", reflect.TypeOf((*MockStorage)(nil).GetRequestBatchStore)) } +// GetRequestLogStore mocks base method. +func (m *MockStorage) GetRequestLogStore() storage.RequestLogStore { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetRequestLogStore") + ret0, _ := ret[0].(storage.RequestLogStore) + return ret0 +} + +// GetRequestLogStore indicates an expected call of GetRequestLogStore. +func (mr *MockStorageMockRecorder) GetRequestLogStore() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRequestLogStore", reflect.TypeOf((*MockStorage)(nil).GetRequestLogStore)) +} + // GetRequestQueueSummaryStore mocks base method. func (m *MockStorage) GetRequestQueueSummaryStore() storage.RequestQueueSummaryStore { m.ctrl.T.Helper() @@ -191,6 +205,34 @@ func (mr *MockStorageMockRecorder) GetRequestStore() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRequestStore", reflect.TypeOf((*MockStorage)(nil).GetRequestStore)) } +// GetRequestSummaryStore mocks base method. +func (m *MockStorage) GetRequestSummaryStore() storage.RequestSummaryStore { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetRequestSummaryStore") + ret0, _ := ret[0].(storage.RequestSummaryStore) + return ret0 +} + +// GetRequestSummaryStore indicates an expected call of GetRequestSummaryStore. +func (mr *MockStorageMockRecorder) GetRequestSummaryStore() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRequestSummaryStore", reflect.TypeOf((*MockStorage)(nil).GetRequestSummaryStore)) +} + +// GetRequestURIStore mocks base method. +func (m *MockStorage) GetRequestURIStore() storage.RequestURIStore { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetRequestURIStore") + ret0, _ := ret[0].(storage.RequestURIStore) + return ret0 +} + +// GetRequestURIStore indicates an expected call of GetRequestURIStore. +func (mr *MockStorageMockRecorder) GetRequestURIStore() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRequestURIStore", reflect.TypeOf((*MockStorage)(nil).GetRequestURIStore)) +} + // GetSpeculationPathSetStore mocks base method. func (m *MockStorage) GetSpeculationPathSetStore() storage.SpeculationPathSetStore { m.ctrl.T.Helper() diff --git a/submitqueue/extension/storage/mysql/request_log_store.go b/submitqueue/extension/storage/mysql/request_log_store.go index ad55673d5..6625a6b9d 100644 --- a/submitqueue/extension/storage/mysql/request_log_store.go +++ b/submitqueue/extension/storage/mysql/request_log_store.go @@ -31,14 +31,17 @@ import ( type requestLogStore struct { db *sql.DB scope tally.Scope + // queue is the queue name this store instance is bound to; every read and + // write is scoped to it. + queue string } // NewRequestLogStore creates a new MySQL-backed RequestLogStore. -func NewRequestLogStore(db *sql.DB, scope tally.Scope) storage.RequestLogStore { - return &requestLogStore{db: db, scope: scope} +func NewRequestLogStore(db *sql.DB, scope tally.Scope, queue string) storage.RequestLogStore { + return &requestLogStore{db: db, scope: scope, queue: queue} } -// Insert appends a new request log record. The primary key is (request_id, timestamp_ms, salt). +// Insert appends a new request log record. The primary key is (queue, request_id, timestamp_ms, salt). // Multiple log entries for the same request can share a timestamp (e.g. concurrent writers or // millisecond-precision collisions), so a random salt is generated to guarantee uniqueness // without requiring the caller to manage deduplication. @@ -46,19 +49,23 @@ func (r *requestLogStore) Insert(ctx context.Context, log entity.RequestLog) (re op := metrics.Begin(r.scope, "insert", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + if log.Queue != r.queue { + return fmt.Errorf("request log request_id=%s queue %q does not match the store's bound queue %q", log.RequestID, log.Queue, r.queue) + } + metadataJSON, err := json.Marshal(log.Metadata) if err != nil { return fmt.Errorf("failed to marshal metadata for request log request_id=%s: %w", log.RequestID, err) } // Generate a random salt to break primary key ties when two inserts share the same - // (request_id, timestamp_ms). The salt is part of the composite primary key in MySQL + // (queue, request_id, timestamp_ms). The salt is part of the composite primary key in MySQL // but is never exposed through the storage interface or returned to callers. salt := rand.Int64() _, err = r.db.ExecContext(ctx, - "INSERT INTO request_log (request_id, timestamp_ms, salt, status, request_version, last_error, metadata) VALUES (?, ?, ?, ?, ?, ?, ?)", - log.RequestID, log.TimestampMs, salt, log.Status, log.RequestVersion, log.LastError, metadataJSON, + "INSERT INTO request_log (queue, request_id, timestamp_ms, salt, status, request_version, last_error, metadata) VALUES (?, ?, ?, ?, ?, ?, ?, ?)", + log.Queue, log.RequestID, log.TimestampMs, salt, log.Status, log.RequestVersion, log.LastError, metadataJSON, ) if err != nil { return fmt.Errorf("failed to insert request log for request_id=%s timestamp_ms=%d: %w", log.RequestID, log.TimestampMs, err) @@ -67,19 +74,20 @@ func (r *requestLogStore) Insert(ctx context.Context, log entity.RequestLog) (re return nil } -// List retrieves all request log records for a given request ID, ordered by timestamp ascending. -// Salt is used as a secondary sort key to provide stable ordering for entries that share a -// timestamp, but it is not included in the SELECT columns and never returned to callers. +// List retrieves all request log records for a given request ID within the bound queue, +// ordered by timestamp ascending. Salt is used as a secondary sort key to provide stable +// ordering for entries that share a timestamp, but it is not included in the SELECT columns +// and never returned to callers. func (r *requestLogStore) List(ctx context.Context, requestID string) (ret []entity.RequestLog, retErr error) { op := metrics.Begin(r.scope, "list", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() rows, err := r.db.QueryContext(ctx, - "SELECT request_id, timestamp_ms, status, request_version, last_error, metadata FROM request_log WHERE request_id = ? ORDER BY timestamp_ms ASC, salt ASC", - requestID, + "SELECT queue, request_id, timestamp_ms, status, request_version, last_error, metadata FROM request_log WHERE queue = ? AND request_id = ? ORDER BY timestamp_ms ASC, salt ASC", + r.queue, requestID, ) if err != nil { - return nil, fmt.Errorf("failed to list request logs for request_id=%s: %w", requestID, err) + return nil, fmt.Errorf("failed to list request logs for queue=%s request_id=%s: %w", r.queue, requestID, err) } defer rows.Close() @@ -88,7 +96,7 @@ func (r *requestLogStore) List(ctx context.Context, requestID string) (ret []ent var log entity.RequestLog var metadataJSON []byte - err := rows.Scan(&log.RequestID, &log.TimestampMs, &log.Status, &log.RequestVersion, &log.LastError, &metadataJSON) + err := rows.Scan(&log.Queue, &log.RequestID, &log.TimestampMs, &log.Status, &log.RequestVersion, &log.LastError, &metadataJSON) if err != nil { return nil, fmt.Errorf("failed to scan request log row for request_id=%s: %w", requestID, err) } @@ -105,7 +113,7 @@ func (r *requestLogStore) List(ctx context.Context, requestID string) (ret []ent } if len(logs) == 0 { - return nil, fmt.Errorf("no request log records for request_id=%s: %w", requestID, storage.ErrNotFound) + return nil, fmt.Errorf("no request log records for queue=%s request_id=%s: %w", r.queue, requestID, storage.ErrNotFound) } return logs, nil diff --git a/submitqueue/extension/storage/mysql/request_log_store_test.go b/submitqueue/extension/storage/mysql/request_log_store_test.go index 577a9ab95..34aa9c41e 100644 --- a/submitqueue/extension/storage/mysql/request_log_store_test.go +++ b/submitqueue/extension/storage/mysql/request_log_store_test.go @@ -28,13 +28,16 @@ import ( "github.com/uber/submitqueue/submitqueue/extension/storage" ) +// testLogQueue is the queue every request-log store in this file is bound to. +const testLogQueue = "monorepo" + func setupRequestLogStoreTest(t *testing.T) (*sql.DB, sqlmock.Sqlmock, storage.RequestLogStore) { t.Helper() db, mock, err := sqlmock.New() require.NoError(t, err) - store := NewRequestLogStore(db, testMetrics()) + store := NewRequestLogStore(db, testMetrics(), testLogQueue) return db, mock, store } @@ -42,6 +45,7 @@ func setupRequestLogStoreTest(t *testing.T) (*sql.DB, sqlmock.Sqlmock, storage.R func TestRequestLogStore_Insert(t *testing.T) { log := entity.RequestLog{ RequestID: "monorepo/1", + Queue: testLogQueue, TimestampMs: 1000, Status: entity.RequestStatusStarted, RequestVersion: 1, @@ -58,7 +62,7 @@ func TestRequestLogStore_Insert(t *testing.T) { name: "success", setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("INSERT INTO request_log"). - WithArgs(log.RequestID, log.TimestampMs, sqlmock.AnyArg(), log.Status, log.RequestVersion, log.LastError, sqlmock.AnyArg()). + WithArgs(log.Queue, log.RequestID, log.TimestampMs, sqlmock.AnyArg(), log.Status, log.RequestVersion, log.LastError, sqlmock.AnyArg()). WillReturnResult(sqlmock.NewResult(0, 1)) }, }, @@ -66,7 +70,7 @@ func TestRequestLogStore_Insert(t *testing.T) { name: "exec error", setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("INSERT INTO request_log"). - WithArgs(log.RequestID, log.TimestampMs, sqlmock.AnyArg(), log.Status, log.RequestVersion, log.LastError, sqlmock.AnyArg()). + WithArgs(log.Queue, log.RequestID, log.TimestampMs, sqlmock.AnyArg(), log.Status, log.RequestVersion, log.LastError, sqlmock.AnyArg()). WillReturnError(fmt.Errorf("connection reset")) }, wantErr: true, @@ -94,6 +98,7 @@ func TestRequestLogStore_Insert(t *testing.T) { func TestRequestLogStore_List(t *testing.T) { log := entity.RequestLog{ RequestID: "monorepo/1", + Queue: testLogQueue, TimestampMs: 1000, Status: entity.RequestStatusStarted, RequestVersion: 1, @@ -113,10 +118,10 @@ func TestRequestLogStore_List(t *testing.T) { name: "found", requestID: log.RequestID, setup: func(mock sqlmock.Sqlmock) { - rows := sqlmock.NewRows([]string{"request_id", "timestamp_ms", "status", "request_version", "last_error", "metadata"}). - AddRow(log.RequestID, log.TimestampMs, string(log.Status), log.RequestVersion, log.LastError, []byte(`{}`)) - mock.ExpectQuery("SELECT request_id, timestamp_ms, status, request_version, last_error, metadata FROM request_log"). - WithArgs(log.RequestID). + rows := sqlmock.NewRows([]string{"queue", "request_id", "timestamp_ms", "status", "request_version", "last_error", "metadata"}). + AddRow(log.Queue, log.RequestID, log.TimestampMs, string(log.Status), log.RequestVersion, log.LastError, []byte(`{}`)) + mock.ExpectQuery("SELECT queue, request_id, timestamp_ms, status, request_version, last_error, metadata FROM request_log"). + WithArgs(testLogQueue, log.RequestID). WillReturnRows(rows) }, want: []entity.RequestLog{log}, @@ -125,9 +130,9 @@ func TestRequestLogStore_List(t *testing.T) { name: "no rows returns ErrNotFound", requestID: "missing", setup: func(mock sqlmock.Sqlmock) { - rows := sqlmock.NewRows([]string{"request_id", "timestamp_ms", "status", "request_version", "last_error", "metadata"}) - mock.ExpectQuery("SELECT request_id, timestamp_ms, status, request_version, last_error, metadata FROM request_log"). - WithArgs("missing"). + rows := sqlmock.NewRows([]string{"queue", "request_id", "timestamp_ms", "status", "request_version", "last_error", "metadata"}) + mock.ExpectQuery("SELECT queue, request_id, timestamp_ms, status, request_version, last_error, metadata FROM request_log"). + WithArgs(testLogQueue, "missing"). WillReturnRows(rows) }, wantErr: true, @@ -137,8 +142,8 @@ func TestRequestLogStore_List(t *testing.T) { name: "query error", requestID: "bad", setup: func(mock sqlmock.Sqlmock) { - mock.ExpectQuery("SELECT request_id, timestamp_ms, status, request_version, last_error, metadata FROM request_log"). - WithArgs("bad"). + mock.ExpectQuery("SELECT queue, request_id, timestamp_ms, status, request_version, last_error, metadata FROM request_log"). + WithArgs(testLogQueue, "bad"). WillReturnError(fmt.Errorf("connection reset")) }, wantErr: true, diff --git a/submitqueue/extension/storage/mysql/request_summary_store.go b/submitqueue/extension/storage/mysql/request_summary_store.go index 3f38cadd4..a84b84546 100644 --- a/submitqueue/extension/storage/mysql/request_summary_store.go +++ b/submitqueue/extension/storage/mysql/request_summary_store.go @@ -32,17 +32,24 @@ import ( type requestSummaryStore struct { db *sql.DB scope tally.Scope + // queue is the queue name this store instance is bound to; every read and + // write is scoped to it. + queue string } // NewRequestSummaryStore creates a MySQL-backed RequestSummaryStore. -func NewRequestSummaryStore(db *sql.DB, scope tally.Scope) storage.RequestSummaryStore { - return &requestSummaryStore{db: db, scope: scope} +func NewRequestSummaryStore(db *sql.DB, scope tally.Scope, queue string) storage.RequestSummaryStore { + return &requestSummaryStore{db: db, scope: scope, queue: queue} } func (s *requestSummaryStore) Create(ctx context.Context, summary entity.RequestSummary) (retErr error) { op := metrics.Begin(s.scope, "create", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + if summary.Queue != s.queue { + return fmt.Errorf("request summary request_id=%s queue %q does not match the store's bound queue %q", summary.RequestID, summary.Queue, s.queue) + } + changeURIsJSON, metadataJSON, err := marshalSummaryJSON(summary.ChangeURIs, summary.Metadata) if err != nil { return fmt.Errorf("failed to marshal request summary metadata request_id=%s: %w", summary.RequestID, err) @@ -50,18 +57,18 @@ func (s *requestSummaryStore) Create(ctx context.Context, summary entity.Request _, err = s.db.ExecContext(ctx, ` INSERT INTO request_summary ( - request_id, queue, change_uris, received_at_ms, status, request_version, + queue, request_id, change_uris, received_at_ms, status, request_version, status_timestamp_ms, version, last_error, metadata ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, - summary.RequestID, summary.Queue, changeURIsJSON, summary.ReceivedAtMs, summary.Status, + summary.Queue, summary.RequestID, changeURIsJSON, summary.ReceivedAtMs, summary.Status, summary.RequestVersion, summary.StatusTimestampMs, summary.Version, summary.LastError, metadataJSON, ) if err != nil { var mysqlErr *mysql.MySQLError if errors.As(err, &mysqlErr) && mysqlErr.Number == mysqlErrDuplicateEntry { - return fmt.Errorf("request summary request_id=%s: %w", summary.RequestID, storage.ErrAlreadyExists) + return fmt.Errorf("request summary queue=%s request_id=%s: %w", summary.Queue, summary.RequestID, storage.ErrAlreadyExists) } return fmt.Errorf("failed to insert request summary request_id=%s: %w", summary.RequestID, err) } @@ -76,13 +83,13 @@ func (s *requestSummaryStore) Get(ctx context.Context, requestID string) (ret en var changeURIsJSON []byte var metadataJSON []byte err := s.db.QueryRowContext(ctx, ` - SELECT request_id, queue, change_uris, received_at_ms, status, request_version, + SELECT queue, request_id, change_uris, received_at_ms, status, request_version, status_timestamp_ms, version, last_error, metadata FROM request_summary - WHERE request_id = ?`, requestID, + WHERE queue = ? AND request_id = ?`, s.queue, requestID, ).Scan( - &ret.RequestID, &ret.Queue, &changeURIsJSON, &ret.ReceivedAtMs, &ret.Status, + &ret.Queue, &ret.RequestID, &changeURIsJSON, &ret.ReceivedAtMs, &ret.Status, &ret.RequestVersion, &ret.StatusTimestampMs, &ret.Version, &ret.LastError, &metadataJSON, ) @@ -90,7 +97,7 @@ func (s *requestSummaryStore) Get(ctx context.Context, requestID string) (ret en return entity.RequestSummary{}, storage.WrapNotFound(err) } if err != nil { - return entity.RequestSummary{}, fmt.Errorf("failed to get request summary request_id=%s: %w", requestID, err) + return entity.RequestSummary{}, fmt.Errorf("failed to get request summary queue=%s request_id=%s: %w", s.queue, requestID, err) } if err := unmarshalSummaryJSON(changeURIsJSON, metadataJSON, &ret.ChangeURIs, &ret.Metadata); err != nil { return entity.RequestSummary{}, fmt.Errorf("failed to decode request summary request_id=%s: %w", requestID, err) @@ -103,6 +110,10 @@ func (s *requestSummaryStore) Update(ctx context.Context, summary entity.Request op := metrics.Begin(s.scope, "update", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + if summary.Queue != s.queue { + return fmt.Errorf("request summary request_id=%s queue %q does not match the store's bound queue %q", summary.RequestID, summary.Queue, s.queue) + } + changeURIsJSON, metadataJSON, err := marshalSummaryJSON(summary.ChangeURIs, summary.Metadata) if err != nil { return fmt.Errorf("failed to marshal request summary request_id=%s: %w", summary.RequestID, err) @@ -110,14 +121,14 @@ func (s *requestSummaryStore) Update(ctx context.Context, summary entity.Request result, err := s.db.ExecContext(ctx, ` UPDATE request_summary - SET queue = ?, change_uris = ?, received_at_ms = ?, status = ?, + SET change_uris = ?, received_at_ms = ?, status = ?, request_version = ?, status_timestamp_ms = ?, version = ?, last_error = ?, metadata = ? - WHERE request_id = ? AND version = ?`, - summary.Queue, changeURIsJSON, summary.ReceivedAtMs, summary.Status, + WHERE queue = ? AND request_id = ? AND version = ?`, + changeURIsJSON, summary.ReceivedAtMs, summary.Status, summary.RequestVersion, summary.StatusTimestampMs, newVersion, summary.LastError, metadataJSON, - summary.RequestID, oldVersion, + summary.Queue, summary.RequestID, oldVersion, ) if err != nil { return fmt.Errorf("failed to update request summary request_id=%s old_version=%d new_version=%d: %w", summary.RequestID, oldVersion, newVersion, err) diff --git a/submitqueue/extension/storage/mysql/request_summary_store_test.go b/submitqueue/extension/storage/mysql/request_summary_store_test.go index de1f03bff..fea7dad1d 100644 --- a/submitqueue/extension/storage/mysql/request_summary_store_test.go +++ b/submitqueue/extension/storage/mysql/request_summary_store_test.go @@ -29,13 +29,16 @@ import ( "github.com/uber/submitqueue/submitqueue/extension/storage" ) +// testSummaryQueue is the queue every request-summary store in this file is bound to. +const testSummaryQueue = "monorepo" + func setupRequestSummaryStoreTest(t *testing.T) (*sql.DB, sqlmock.Sqlmock, storage.RequestSummaryStore) { t.Helper() db, mock, err := sqlmock.New() require.NoError(t, err) - store := NewRequestSummaryStore(db, testMetrics()) + store := NewRequestSummaryStore(db, testMetrics(), testSummaryQueue) return db, mock, store } @@ -43,7 +46,7 @@ func setupRequestSummaryStoreTest(t *testing.T) (*sql.DB, sqlmock.Sqlmock, stora func TestRequestSummaryStore_Create(t *testing.T) { summary := entity.RequestSummary{ RequestID: "monorepo/1", - Queue: "monorepo", + Queue: testSummaryQueue, ChangeURIs: []string{"github://github.example.com/uber/submitqueue/pull/123/deadbeef"}, ReceivedAtMs: 1000, Status: entity.RequestStatusStarted, @@ -64,7 +67,7 @@ func TestRequestSummaryStore_Create(t *testing.T) { name: "success", setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("INSERT INTO request_summary"). - WithArgs(summary.RequestID, summary.Queue, sqlmock.AnyArg(), summary.ReceivedAtMs, summary.Status, + WithArgs(summary.Queue, summary.RequestID, sqlmock.AnyArg(), summary.ReceivedAtMs, summary.Status, summary.RequestVersion, summary.StatusTimestampMs, summary.Version, summary.LastError, sqlmock.AnyArg()). WillReturnResult(sqlmock.NewResult(0, 1)) }, @@ -73,7 +76,7 @@ func TestRequestSummaryStore_Create(t *testing.T) { name: "duplicate request id returns ErrAlreadyExists", setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("INSERT INTO request_summary"). - WithArgs(summary.RequestID, summary.Queue, sqlmock.AnyArg(), summary.ReceivedAtMs, summary.Status, + WithArgs(summary.Queue, summary.RequestID, sqlmock.AnyArg(), summary.ReceivedAtMs, summary.Status, summary.RequestVersion, summary.StatusTimestampMs, summary.Version, summary.LastError, sqlmock.AnyArg()). WillReturnError(&mysql.MySQLError{Number: mysqlErrDuplicateEntry}) }, @@ -84,7 +87,7 @@ func TestRequestSummaryStore_Create(t *testing.T) { name: "other exec error", setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("INSERT INTO request_summary"). - WithArgs(summary.RequestID, summary.Queue, sqlmock.AnyArg(), summary.ReceivedAtMs, summary.Status, + WithArgs(summary.Queue, summary.RequestID, sqlmock.AnyArg(), summary.ReceivedAtMs, summary.Status, summary.RequestVersion, summary.StatusTimestampMs, summary.Version, summary.LastError, sqlmock.AnyArg()). WillReturnError(fmt.Errorf("connection reset")) }, @@ -116,7 +119,7 @@ func TestRequestSummaryStore_Create(t *testing.T) { func TestRequestSummaryStore_Get(t *testing.T) { want := entity.RequestSummary{ RequestID: "monorepo/1", - Queue: "monorepo", + Queue: testSummaryQueue, ChangeURIs: []string{"github://github.example.com/uber/submitqueue/pull/123/deadbeef"}, ReceivedAtMs: 1000, Status: entity.RequestStatusStarted, @@ -140,13 +143,13 @@ func TestRequestSummaryStore_Get(t *testing.T) { requestID: want.RequestID, setup: func(mock sqlmock.Sqlmock) { rows := sqlmock.NewRows([]string{ - "request_id", "queue", "change_uris", "received_at_ms", "status", + "queue", "request_id", "change_uris", "received_at_ms", "status", "request_version", "status_timestamp_ms", "version", "last_error", "metadata", - }).AddRow(want.RequestID, want.Queue, []byte(`["github://github.example.com/uber/submitqueue/pull/123/deadbeef"]`), + }).AddRow(want.Queue, want.RequestID, []byte(`["github://github.example.com/uber/submitqueue/pull/123/deadbeef"]`), want.ReceivedAtMs, string(want.Status), want.RequestVersion, want.StatusTimestampMs, want.Version, want.LastError, []byte(`{"key":"value"}`)) - mock.ExpectQuery("SELECT request_id, queue, change_uris, received_at_ms, status"). - WithArgs(want.RequestID). + mock.ExpectQuery("SELECT queue, request_id, change_uris, received_at_ms, status"). + WithArgs(testSummaryQueue, want.RequestID). WillReturnRows(rows) }, want: want, @@ -155,8 +158,8 @@ func TestRequestSummaryStore_Get(t *testing.T) { name: "not found", requestID: "missing", setup: func(mock sqlmock.Sqlmock) { - mock.ExpectQuery("SELECT request_id, queue, change_uris, received_at_ms, status"). - WithArgs("missing"). + mock.ExpectQuery("SELECT queue, request_id, change_uris, received_at_ms, status"). + WithArgs(testSummaryQueue, "missing"). WillReturnError(sql.ErrNoRows) }, wantErr: true, @@ -166,8 +169,8 @@ func TestRequestSummaryStore_Get(t *testing.T) { name: "query error", requestID: "bad", setup: func(mock sqlmock.Sqlmock) { - mock.ExpectQuery("SELECT request_id, queue, change_uris, received_at_ms, status"). - WithArgs("bad"). + mock.ExpectQuery("SELECT queue, request_id, change_uris, received_at_ms, status"). + WithArgs(testSummaryQueue, "bad"). WillReturnError(fmt.Errorf("connection reset")) }, wantErr: true, @@ -199,7 +202,7 @@ func TestRequestSummaryStore_Get(t *testing.T) { func TestRequestSummaryStore_Update(t *testing.T) { summary := entity.RequestSummary{ RequestID: "monorepo/1", - Queue: "monorepo-updated", + Queue: testSummaryQueue, ChangeURIs: []string{"github://github.example.com/uber/submitqueue/pull/456/cafebabe"}, ReceivedAtMs: 1500, Status: entity.RequestStatusValidated, @@ -223,9 +226,9 @@ func TestRequestSummaryStore_Update(t *testing.T) { summary: summary, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE request_summary"). - WithArgs(summary.Queue, []byte(`["github://github.example.com/uber/submitqueue/pull/456/cafebabe"]`), + WithArgs([]byte(`["github://github.example.com/uber/submitqueue/pull/456/cafebabe"]`), summary.ReceivedAtMs, summary.Status, summary.RequestVersion, summary.StatusTimestampMs, - newVersion, summary.LastError, []byte(`{"result":"validated"}`), summary.RequestID, oldVersion). + newVersion, summary.LastError, []byte(`{"result":"validated"}`), summary.Queue, summary.RequestID, oldVersion). WillReturnResult(sqlmock.NewResult(0, 1)) }, }, @@ -234,9 +237,9 @@ func TestRequestSummaryStore_Update(t *testing.T) { summary: summary, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE request_summary"). - WithArgs(summary.Queue, []byte(`["github://github.example.com/uber/submitqueue/pull/456/cafebabe"]`), + WithArgs([]byte(`["github://github.example.com/uber/submitqueue/pull/456/cafebabe"]`), summary.ReceivedAtMs, summary.Status, summary.RequestVersion, summary.StatusTimestampMs, - newVersion, summary.LastError, []byte(`{"result":"validated"}`), summary.RequestID, oldVersion). + newVersion, summary.LastError, []byte(`{"result":"validated"}`), summary.Queue, summary.RequestID, oldVersion). WillReturnResult(sqlmock.NewResult(0, 0)) }, wantErr: true, @@ -247,9 +250,9 @@ func TestRequestSummaryStore_Update(t *testing.T) { summary: summary, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE request_summary"). - WithArgs(summary.Queue, []byte(`["github://github.example.com/uber/submitqueue/pull/456/cafebabe"]`), + WithArgs([]byte(`["github://github.example.com/uber/submitqueue/pull/456/cafebabe"]`), summary.ReceivedAtMs, summary.Status, summary.RequestVersion, summary.StatusTimestampMs, - newVersion, summary.LastError, []byte(`{"result":"validated"}`), summary.RequestID, oldVersion). + newVersion, summary.LastError, []byte(`{"result":"validated"}`), summary.Queue, summary.RequestID, oldVersion). WillReturnError(fmt.Errorf("connection reset")) }, wantErr: true, @@ -259,9 +262,9 @@ func TestRequestSummaryStore_Update(t *testing.T) { summary: summary, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE request_summary"). - WithArgs(summary.Queue, []byte(`["github://github.example.com/uber/submitqueue/pull/456/cafebabe"]`), + WithArgs([]byte(`["github://github.example.com/uber/submitqueue/pull/456/cafebabe"]`), summary.ReceivedAtMs, summary.Status, summary.RequestVersion, summary.StatusTimestampMs, - newVersion, summary.LastError, []byte(`{"result":"validated"}`), summary.RequestID, oldVersion). + newVersion, summary.LastError, []byte(`{"result":"validated"}`), summary.Queue, summary.RequestID, oldVersion). WillReturnResult(sqlmock.NewErrorResult(fmt.Errorf("rows unavailable"))) }, wantErr: true, @@ -280,9 +283,9 @@ func TestRequestSummaryStore_Update(t *testing.T) { }, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE request_summary"). - WithArgs(summary.Queue, []byte(`[]`), summary.ReceivedAtMs, summary.Status, + WithArgs([]byte(`[]`), summary.ReceivedAtMs, summary.Status, summary.RequestVersion, summary.StatusTimestampMs, newVersion, summary.LastError, - []byte(`{}`), summary.RequestID, oldVersion). + []byte(`{}`), summary.Queue, summary.RequestID, oldVersion). WillReturnResult(sqlmock.NewResult(0, 1)) }, }, diff --git a/submitqueue/extension/storage/mysql/request_uri_store.go b/submitqueue/extension/storage/mysql/request_uri_store.go index b38a5d69f..220d6c736 100644 --- a/submitqueue/extension/storage/mysql/request_uri_store.go +++ b/submitqueue/extension/storage/mysql/request_uri_store.go @@ -31,25 +31,32 @@ import ( type requestURIStore struct { db *sql.DB scope tally.Scope + // queue is the queue name this store instance is bound to; every read and + // write is scoped to it. + queue string } // NewRequestURIStore creates a MySQL-backed RequestURIStore. -func NewRequestURIStore(db *sql.DB, scope tally.Scope) storage.RequestURIStore { - return &requestURIStore{db: db, scope: scope} +func NewRequestURIStore(db *sql.DB, scope tally.Scope, queue string) storage.RequestURIStore { + return &requestURIStore{db: db, scope: scope, queue: queue} } func (s *requestURIStore) Create(ctx context.Context, mapping entity.RequestURI) (retErr error) { op := metrics.Begin(s.scope, "create", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + if mapping.Queue != s.queue { + return fmt.Errorf("request URI change_uri=%s queue %q does not match the store's bound queue %q", mapping.ChangeURI, mapping.Queue, s.queue) + } + _, err := s.db.ExecContext(ctx, - "INSERT INTO change_uri_request_mapping (change_uri, received_at_ms, request_id) VALUES (?, ?, ?)", - mapping.ChangeURI, mapping.ReceivedAtMs, mapping.RequestID, + "INSERT INTO change_uri_request_mapping (queue, change_uri, received_at_ms, request_id) VALUES (?, ?, ?, ?)", + mapping.Queue, mapping.ChangeURI, mapping.ReceivedAtMs, mapping.RequestID, ) if err != nil { var mysqlErr *mysql.MySQLError if errors.As(err, &mysqlErr) && mysqlErr.Number == mysqlErrDuplicateEntry { - return fmt.Errorf("request URI change_uri=%s received_at_ms=%d request_id=%s: %w", mapping.ChangeURI, mapping.ReceivedAtMs, mapping.RequestID, storage.ErrAlreadyExists) + return fmt.Errorf("request URI queue=%s change_uri=%s received_at_ms=%d request_id=%s: %w", mapping.Queue, mapping.ChangeURI, mapping.ReceivedAtMs, mapping.RequestID, storage.ErrAlreadyExists) } return fmt.Errorf("failed to insert request URI request_id=%s change_uri=%s: %w", mapping.RequestID, mapping.ChangeURI, err) } @@ -61,20 +68,20 @@ func (s *requestURIStore) ListByURI(ctx context.Context, changeURI string, limit defer func() { op.Complete(retErr) }() rows, err := s.db.QueryContext(ctx, ` - SELECT change_uri, received_at_ms, request_id + SELECT queue, change_uri, received_at_ms, request_id FROM change_uri_request_mapping - WHERE change_uri = ? + WHERE queue = ? AND change_uri = ? ORDER BY received_at_ms DESC, request_id DESC - LIMIT ?`, changeURI, limit) + LIMIT ?`, s.queue, changeURI, limit) if err != nil { - return nil, fmt.Errorf("failed to list request URIs change_uri=%s: %w", changeURI, err) + return nil, fmt.Errorf("failed to list request URIs queue=%s change_uri=%s: %w", s.queue, changeURI, err) } defer rows.Close() results := make([]entity.RequestURI, 0) for rows.Next() { var mapping entity.RequestURI - if err := rows.Scan(&mapping.ChangeURI, &mapping.ReceivedAtMs, &mapping.RequestID); err != nil { + if err := rows.Scan(&mapping.Queue, &mapping.ChangeURI, &mapping.ReceivedAtMs, &mapping.RequestID); err != nil { return nil, fmt.Errorf("failed to scan request URI change_uri=%s: %w", changeURI, err) } results = append(results, mapping) diff --git a/submitqueue/extension/storage/mysql/request_uri_store_test.go b/submitqueue/extension/storage/mysql/request_uri_store_test.go index 6e01e71bd..cc4734901 100644 --- a/submitqueue/extension/storage/mysql/request_uri_store_test.go +++ b/submitqueue/extension/storage/mysql/request_uri_store_test.go @@ -29,13 +29,16 @@ import ( "github.com/uber/submitqueue/submitqueue/extension/storage" ) +// testURIQueue is the queue every request-URI store in this file is bound to. +const testURIQueue = "monorepo" + func setupRequestURIStoreTest(t *testing.T) (*sql.DB, sqlmock.Sqlmock, storage.RequestURIStore) { t.Helper() db, mock, err := sqlmock.New() require.NoError(t, err) - store := NewRequestURIStore(db, testMetrics()) + store := NewRequestURIStore(db, testMetrics(), testURIQueue) return db, mock, store } @@ -43,6 +46,7 @@ func setupRequestURIStoreTest(t *testing.T) (*sql.DB, sqlmock.Sqlmock, storage.R func TestRequestURIStore_Create(t *testing.T) { mapping := entity.RequestURI{ ChangeURI: "github://github.example.com/uber/submitqueue/pull/123/deadbeef", + Queue: testURIQueue, ReceivedAtMs: 1000, RequestID: "monorepo/1", } @@ -57,7 +61,7 @@ func TestRequestURIStore_Create(t *testing.T) { name: "success", setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("INSERT INTO change_uri_request_mapping"). - WithArgs(mapping.ChangeURI, mapping.ReceivedAtMs, mapping.RequestID). + WithArgs(mapping.Queue, mapping.ChangeURI, mapping.ReceivedAtMs, mapping.RequestID). WillReturnResult(sqlmock.NewResult(0, 1)) }, }, @@ -65,7 +69,7 @@ func TestRequestURIStore_Create(t *testing.T) { name: "duplicate mapping returns ErrAlreadyExists", setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("INSERT INTO change_uri_request_mapping"). - WithArgs(mapping.ChangeURI, mapping.ReceivedAtMs, mapping.RequestID). + WithArgs(mapping.Queue, mapping.ChangeURI, mapping.ReceivedAtMs, mapping.RequestID). WillReturnError(&mysql.MySQLError{Number: mysqlErrDuplicateEntry}) }, wantErr: true, @@ -75,7 +79,7 @@ func TestRequestURIStore_Create(t *testing.T) { name: "other exec error", setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("INSERT INTO change_uri_request_mapping"). - WithArgs(mapping.ChangeURI, mapping.ReceivedAtMs, mapping.RequestID). + WithArgs(mapping.Queue, mapping.ChangeURI, mapping.ReceivedAtMs, mapping.RequestID). WillReturnError(fmt.Errorf("connection reset")) }, wantErr: true, @@ -106,6 +110,7 @@ func TestRequestURIStore_Create(t *testing.T) { func TestRequestURIStore_ListByURI(t *testing.T) { mapping := entity.RequestURI{ ChangeURI: "github://github.example.com/uber/submitqueue/pull/123/deadbeef", + Queue: testURIQueue, ReceivedAtMs: 1000, RequestID: "monorepo/1", } @@ -119,10 +124,10 @@ func TestRequestURIStore_ListByURI(t *testing.T) { { name: "found", setup: func(mock sqlmock.Sqlmock) { - rows := sqlmock.NewRows([]string{"change_uri", "received_at_ms", "request_id"}). - AddRow(mapping.ChangeURI, mapping.ReceivedAtMs, mapping.RequestID) - mock.ExpectQuery("SELECT change_uri, received_at_ms, request_id"). - WithArgs(mapping.ChangeURI, 10). + rows := sqlmock.NewRows([]string{"queue", "change_uri", "received_at_ms", "request_id"}). + AddRow(mapping.Queue, mapping.ChangeURI, mapping.ReceivedAtMs, mapping.RequestID) + mock.ExpectQuery("SELECT queue, change_uri, received_at_ms, request_id"). + WithArgs(testURIQueue, mapping.ChangeURI, 10). WillReturnRows(rows) }, want: []entity.RequestURI{mapping}, @@ -130,9 +135,9 @@ func TestRequestURIStore_ListByURI(t *testing.T) { { name: "no rows returns empty slice", setup: func(mock sqlmock.Sqlmock) { - rows := sqlmock.NewRows([]string{"change_uri", "received_at_ms", "request_id"}) - mock.ExpectQuery("SELECT change_uri, received_at_ms, request_id"). - WithArgs(mapping.ChangeURI, 10). + rows := sqlmock.NewRows([]string{"queue", "change_uri", "received_at_ms", "request_id"}) + mock.ExpectQuery("SELECT queue, change_uri, received_at_ms, request_id"). + WithArgs(testURIQueue, mapping.ChangeURI, 10). WillReturnRows(rows) }, want: []entity.RequestURI{}, @@ -140,8 +145,8 @@ func TestRequestURIStore_ListByURI(t *testing.T) { { name: "query error", setup: func(mock sqlmock.Sqlmock) { - mock.ExpectQuery("SELECT change_uri, received_at_ms, request_id"). - WithArgs(mapping.ChangeURI, 10). + mock.ExpectQuery("SELECT queue, change_uri, received_at_ms, request_id"). + WithArgs(testURIQueue, mapping.ChangeURI, 10). WillReturnError(fmt.Errorf("connection reset")) }, wantErr: true, diff --git a/submitqueue/extension/storage/mysql/schema/README.md b/submitqueue/extension/storage/mysql/schema/README.md index 2240a7e18..59fef6c4b 100644 --- a/submitqueue/extension/storage/mysql/schema/README.md +++ b/submitqueue/extension/storage/mysql/schema/README.md @@ -2,9 +2,11 @@ ## Queue-leading primary keys -Every queue-scoped table leads its primary key with `queue`: `request` and `batch` on `(queue, id)`, `build` on `(queue, id)`, `batch_dependent` on `(queue, batch_id)`, `request_batch` on `(queue, request_id, batch_id)`, `change` on `(queue, uri, request_id)`, `queue_batch_state` on `(queue, state, batch_id)`, and `request_summary_by_queue` on `(queue, received_at_ms, request_id)`. A queue-bound store instance prefixes every read and stamps every write with its bound queue, so one queue's rows are unreachable through another queue's binding and the tables are shardable by queue. The `build` key also removes a cross-queue uniqueness assumption: build IDs are runner-minted, so two queues sharing one CI pipeline may legitimately mint the same identifier. +Every table leads its primary key with `queue`: `request` and `batch` on `(queue, id)`, `build` on `(queue, id)`, `batch_dependent` on `(queue, batch_id)`, `request_batch` on `(queue, request_id, batch_id)`, `change` on `(queue, uri, request_id)`, `queue_batch_state` on `(queue, state, batch_id)`, `speculation_path_set` on `(queue, head)`, `request_summary` on `(queue, request_id)`, `request_log` on `(queue, request_id, timestamp_ms, salt)`, `change_uri_request_mapping` on `(queue, change_uri, received_at_ms, request_id)`, and `request_summary_by_queue` on `(queue, received_at_ms, request_id)`. A queue-bound store instance prefixes every read and stamps every write with its bound queue, so one queue's rows are unreachable through another queue's binding and every table is shardable by queue. `//tool/linter/queueshard` enforces this, and also rejects any secondary index that does not itself lead with `queue`, since such an index would reintroduce a cross-queue access path. -The global read-model tables (`request_summary`, `request_log`, `change_uri_request_mapping`) keep queue-free keys — their lookups start from identifiers that arrive without queue context. +The `build` key also removes a cross-queue uniqueness assumption: build IDs are runner-minted, so two queues sharing one CI pipeline may legitimately mint the same identifier. `speculation_path_set` relies on the same property for its head: a batch ID is unique only within its queue. + +Because the queue is part of every key, the request identifier alone no longer addresses a row — the read APIs take the queue alongside the sqid or change URI, and the stores are resolved per queue through `storage.Factory`. No identifier is parsed to recover a queue. ## batch table @@ -32,7 +34,7 @@ The gateway request read model uses three additive tables and requires no altera ### `request_summary` -`request_summary` is keyed by `request_id` and serves direct Status lookup. It stores immutable receipt context plus the current materialized request-log winner and its optimistic-lock projection version. +`request_summary` is keyed by `(queue, request_id)` and serves direct Status lookup within one queue. It stores immutable receipt context plus the current materialized request-log winner and its optimistic-lock projection version. ### `request_summary_by_queue` @@ -40,7 +42,11 @@ The gateway request read model uses three additive tables and requires no altera ### `change_uri_request_mapping` -`change_uri_request_mapping` is keyed by `(change_uri, received_at_ms, request_id)` and serves bounded newest-first Status lookup by change URI. The gateway reads at most 101 mappings to enforce the API maximum of 100 results without silently truncating. +`change_uri_request_mapping` is keyed by `(queue, change_uri, received_at_ms, request_id)` and serves bounded newest-first Status lookup by change URI within one queue. The gateway reads at most 101 mappings to enforce the API maximum of 100 results without silently truncating. A change URI landed into several queues has independent mappings in each, so looking it up across queues is one call per queue. + +### `request_log` + +`request_log` is keyed by `(queue, request_id, timestamp_ms, salt)` and holds the append-only audit trail behind History. `salt` disambiguates entries sharing a request, queue and millisecond; it is part of the key but never exposed through the storage interface. ### JSON collections diff --git a/submitqueue/extension/storage/mysql/schema/change_uri_request_mapping.sql b/submitqueue/extension/storage/mysql/schema/change_uri_request_mapping.sql index 12225282b..89c7768a3 100644 --- a/submitqueue/extension/storage/mysql/schema/change_uri_request_mapping.sql +++ b/submitqueue/extension/storage/mysql/schema/change_uri_request_mapping.sql @@ -1,6 +1,11 @@ +-- change_uri_request_mapping is the reverse index from a change URI to the requests that +-- claimed it, serving bounded newest-first Status lookup by change URI. queue leads the PK so +-- the table is shardable by queue: a change URI landed into several queues is looked up one +-- queue at a time, and each queue's mappings are unreachable through another queue's binding. CREATE TABLE IF NOT EXISTS change_uri_request_mapping ( + queue VARCHAR(255) NOT NULL, change_uri VARCHAR(255) NOT NULL, received_at_ms BIGINT NOT NULL, request_id VARCHAR(255) NOT NULL, - PRIMARY KEY (change_uri, received_at_ms, request_id) + PRIMARY KEY (queue, change_uri, received_at_ms, request_id) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; diff --git a/submitqueue/extension/storage/mysql/schema/request_log.sql b/submitqueue/extension/storage/mysql/schema/request_log.sql index 8866eeacd..261e76cd1 100644 --- a/submitqueue/extension/storage/mysql/schema/request_log.sql +++ b/submitqueue/extension/storage/mysql/schema/request_log.sql @@ -1,4 +1,8 @@ +-- request_log is the append-only audit trail of request status transitions. queue leads the +-- PK so the table is shardable by queue; request_id is unique only within its queue. salt +-- disambiguates entries sharing a request, queue and millisecond. CREATE TABLE IF NOT EXISTS request_log ( + queue VARCHAR(255) NOT NULL, request_id VARCHAR(255) NOT NULL, timestamp_ms BIGINT NOT NULL, salt BIGINT NOT NULL, @@ -6,5 +10,5 @@ CREATE TABLE IF NOT EXISTS request_log ( request_version INT NOT NULL, last_error TEXT NOT NULL, metadata JSON NOT NULL, - PRIMARY KEY (request_id, timestamp_ms, salt) + PRIMARY KEY (queue, request_id, timestamp_ms, salt) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; diff --git a/submitqueue/extension/storage/mysql/schema/request_summary.sql b/submitqueue/extension/storage/mysql/schema/request_summary.sql index d4bcce7d8..f06c04ad8 100644 --- a/submitqueue/extension/storage/mysql/schema/request_summary.sql +++ b/submitqueue/extension/storage/mysql/schema/request_summary.sql @@ -1,6 +1,9 @@ +-- request_summary is the authoritative per-request materialized view serving direct Status +-- lookup. queue leads the PK so the table is shardable by queue; request_id is unique only +-- within its queue. CREATE TABLE IF NOT EXISTS request_summary ( - request_id VARCHAR(255) NOT NULL, queue VARCHAR(255) NOT NULL, + request_id VARCHAR(255) NOT NULL, change_uris JSON NOT NULL, received_at_ms BIGINT NOT NULL, status VARCHAR(64) NOT NULL, @@ -9,5 +12,5 @@ CREATE TABLE IF NOT EXISTS request_summary ( version INT NOT NULL, last_error TEXT NOT NULL, metadata JSON NOT NULL, - PRIMARY KEY (request_id) + PRIMARY KEY (queue, request_id) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; diff --git a/submitqueue/extension/storage/mysql/schema/speculation_path_set.sql b/submitqueue/extension/storage/mysql/schema/speculation_path_set.sql index 32dd28137..59ab01094 100644 --- a/submitqueue/extension/storage/mysql/schema/speculation_path_set.sql +++ b/submitqueue/extension/storage/mysql/schema/speculation_path_set.sql @@ -1,6 +1,10 @@ +-- speculation_path_set holds one head batch's chosen speculation paths, versioned as a whole. +-- queue leads the PK so the table is shardable by queue; head is the head batch's ID, stored +-- opaquely and never parsed. CREATE TABLE IF NOT EXISTS speculation_path_set ( + queue VARCHAR(255) NOT NULL, head VARCHAR(255) NOT NULL, paths JSON NOT NULL, version INT NOT NULL, - PRIMARY KEY (head) + PRIMARY KEY (queue, head) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; diff --git a/submitqueue/extension/storage/mysql/speculation_path_set_store.go b/submitqueue/extension/storage/mysql/speculation_path_set_store.go index 2db498487..6a74733dc 100644 --- a/submitqueue/extension/storage/mysql/speculation_path_set_store.go +++ b/submitqueue/extension/storage/mysql/speculation_path_set_store.go @@ -32,11 +32,14 @@ import ( type speculationPathSetStore struct { db *sql.DB scope tally.Scope + // queue is the queue name this store instance is bound to; every read and + // write is scoped to it. + queue string } // NewSpeculationPathSetStore creates a new MySQL-backed SpeculationPathSetStore. -func NewSpeculationPathSetStore(db *sql.DB, scope tally.Scope) storage.SpeculationPathSetStore { - return &speculationPathSetStore{db: db, scope: scope} +func NewSpeculationPathSetStore(db *sql.DB, scope tally.Scope, queue string) storage.SpeculationPathSetStore { + return &speculationPathSetStore{db: db, scope: scope, queue: queue} } // Get retrieves a head's path set, where head is the head batch's ID. @@ -49,19 +52,19 @@ func (s *speculationPathSetStore) Get(ctx context.Context, head string) (ret ent var pathsJSON []byte err := s.db.QueryRowContext(ctx, - "SELECT head, paths, version FROM speculation_path_set WHERE head = ?", - head, - ).Scan(&set.Head, &pathsJSON, &set.Version) + "SELECT queue, head, paths, version FROM speculation_path_set WHERE queue = ? AND head = ?", + s.queue, head, + ).Scan(&set.Queue, &set.Head, &pathsJSON, &set.Version) if errors.Is(err, sql.ErrNoRows) { return entity.SpeculationPathSet{}, storage.WrapNotFound(err) } if err != nil { - return entity.SpeculationPathSet{}, fmt.Errorf("failed to get speculation path set entity head=%s from the database: %w", head, err) + return entity.SpeculationPathSet{}, fmt.Errorf("failed to get speculation path set entity queue=%s head=%s from the database: %w", s.queue, head, err) } if err := json.Unmarshal(pathsJSON, &set.Paths); err != nil { - return entity.SpeculationPathSet{}, fmt.Errorf("failed to unmarshal paths for speculation path set entity head=%s from the database: %w", head, err) + return entity.SpeculationPathSet{}, fmt.Errorf("failed to unmarshal paths for speculation path set entity queue=%s head=%s from the database: %w", s.queue, head, err) } return set, nil @@ -72,21 +75,25 @@ func (s *speculationPathSetStore) Create(ctx context.Context, set entity.Specula op := metrics.Begin(s.scope, "create", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + if set.Queue != s.queue { + return fmt.Errorf("speculation path set head=%s queue %q does not match the store's bound queue %q", set.Head, set.Queue, s.queue) + } + pathsJSON, err := json.Marshal(set.Paths) if err != nil { return fmt.Errorf("failed to marshal paths head=%s for Create speculation path set entity: %w", set.Head, err) } _, err = s.db.ExecContext(ctx, - "INSERT INTO speculation_path_set (head, paths, version) VALUES (?, ?, ?)", - set.Head, pathsJSON, set.Version, + "INSERT INTO speculation_path_set (queue, head, paths, version) VALUES (?, ?, ?, ?)", + set.Queue, set.Head, pathsJSON, set.Version, ) if err != nil { var mysqlErr *mysql.MySQLError if errors.As(err, &mysqlErr) && mysqlErr.Number == mysqlErrDuplicateEntry { - return fmt.Errorf("speculation path set entity head=%s: %w", set.Head, storage.ErrAlreadyExists) + return fmt.Errorf("speculation path set entity queue=%s head=%s: %w", set.Queue, set.Head, storage.ErrAlreadyExists) } - return fmt.Errorf("failed to insert speculation path set entity head=%s: %w", set.Head, err) + return fmt.Errorf("failed to insert speculation path set entity queue=%s head=%s: %w", set.Queue, set.Head, err) } return nil @@ -99,34 +106,38 @@ func (s *speculationPathSetStore) Update(ctx context.Context, set entity.Specula op := metrics.Begin(s.scope, "update", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + if set.Queue != s.queue { + return fmt.Errorf("speculation path set head=%s queue %q does not match the store's bound queue %q", set.Head, set.Queue, s.queue) + } + pathsJSON, err := json.Marshal(set.Paths) if err != nil { return fmt.Errorf("failed to marshal paths head=%s for Update speculation path set entity: %w", set.Head, err) } result, err := s.db.ExecContext(ctx, - "UPDATE speculation_path_set SET paths = ?, version = ? WHERE head = ? AND version = ?", - pathsJSON, newVersion, set.Head, oldVersion, + "UPDATE speculation_path_set SET paths = ?, version = ? WHERE queue = ? AND head = ? AND version = ?", + pathsJSON, newVersion, set.Queue, set.Head, oldVersion, ) if err != nil { return fmt.Errorf( - "failed to update speculation path set for head=%q oldVersion=%d newVersion=%d: %w", - set.Head, oldVersion, newVersion, err, + "failed to update speculation path set for queue=%q head=%q oldVersion=%d newVersion=%d: %w", + set.Queue, set.Head, oldVersion, newVersion, err, ) } rowsAffected, err := result.RowsAffected() if err != nil { return fmt.Errorf( - "failed to get rows affected from update for head=%q oldVersion=%d newVersion=%d: %w", - set.Head, oldVersion, newVersion, err, + "failed to get rows affected from update for queue=%q head=%q oldVersion=%d newVersion=%d: %w", + set.Queue, set.Head, oldVersion, newVersion, err, ) } if rowsAffected != 1 { return fmt.Errorf( - "version mismatch for speculation path set update: head=%q expected_version=%d: %w", - set.Head, oldVersion, storage.ErrVersionMismatch, + "version mismatch for speculation path set update: queue=%q head=%q expected_version=%d: %w", + set.Queue, set.Head, oldVersion, storage.ErrVersionMismatch, ) } diff --git a/submitqueue/extension/storage/mysql/speculation_path_set_store_test.go b/submitqueue/extension/storage/mysql/speculation_path_set_store_test.go index 9b39ace04..d5c6588d3 100644 --- a/submitqueue/extension/storage/mysql/speculation_path_set_store_test.go +++ b/submitqueue/extension/storage/mysql/speculation_path_set_store_test.go @@ -30,13 +30,16 @@ import ( "github.com/uber/submitqueue/submitqueue/extension/storage" ) +// testSpecQueue is the queue every speculation-path-set store in this file is bound to. +const testSpecQueue = "monorepo" + func setupSpeculationPathSetStoreTest(t *testing.T) (*sql.DB, sqlmock.Sqlmock, storage.SpeculationPathSetStore) { t.Helper() db, mock, err := sqlmock.New() require.NoError(t, err) - store := NewSpeculationPathSetStore(db, testMetrics()) + store := NewSpeculationPathSetStore(db, testMetrics(), testSpecQueue) return db, mock, store } @@ -54,7 +57,8 @@ func testPathSet(head string) entity.SpeculationPathSet { Dependencies: []entity.PathDependency{{Batch: dep, Assumption: entity.DependencyAssumptionFails}}, } return entity.SpeculationPathSet{ - Head: head, + Queue: testSpecQueue, + Head: head, Paths: []entity.SpeculationPathEntry{ { ID: succeeds.ID(), @@ -96,10 +100,10 @@ func TestSpeculationPathSetStore_Get(t *testing.T) { name: "found", head: want.Head, setup: func(mock sqlmock.Sqlmock) { - rows := sqlmock.NewRows([]string{"head", "paths", "version"}). - AddRow(want.Head, pathsJSON, want.Version) - mock.ExpectQuery("SELECT head, paths, version FROM speculation_path_set"). - WithArgs(want.Head). + rows := sqlmock.NewRows([]string{"queue", "head", "paths", "version"}). + AddRow(want.Queue, want.Head, pathsJSON, want.Version) + mock.ExpectQuery("SELECT queue, head, paths, version FROM speculation_path_set"). + WithArgs(testSpecQueue, want.Head). WillReturnRows(rows) }, want: want, @@ -108,8 +112,8 @@ func TestSpeculationPathSetStore_Get(t *testing.T) { name: "not found", head: "missing", setup: func(mock sqlmock.Sqlmock) { - mock.ExpectQuery("SELECT head, paths, version FROM speculation_path_set"). - WithArgs("missing"). + mock.ExpectQuery("SELECT queue, head, paths, version FROM speculation_path_set"). + WithArgs(testSpecQueue, "missing"). WillReturnError(sql.ErrNoRows) }, wantErr: true, @@ -119,10 +123,10 @@ func TestSpeculationPathSetStore_Get(t *testing.T) { name: "malformed paths json", head: "corrupt", setup: func(mock sqlmock.Sqlmock) { - rows := sqlmock.NewRows([]string{"head", "paths", "version"}). - AddRow("corrupt", []byte("{not json"), 1) - mock.ExpectQuery("SELECT head, paths, version FROM speculation_path_set"). - WithArgs("corrupt"). + rows := sqlmock.NewRows([]string{"queue", "head", "paths", "version"}). + AddRow(testSpecQueue, "corrupt", []byte("{not json"), 1) + mock.ExpectQuery("SELECT queue, head, paths, version FROM speculation_path_set"). + WithArgs(testSpecQueue, "corrupt"). WillReturnRows(rows) }, wantErr: true, @@ -131,8 +135,8 @@ func TestSpeculationPathSetStore_Get(t *testing.T) { name: "query error", head: "bad", setup: func(mock sqlmock.Sqlmock) { - mock.ExpectQuery("SELECT head, paths, version FROM speculation_path_set"). - WithArgs("bad"). + mock.ExpectQuery("SELECT queue, head, paths, version FROM speculation_path_set"). + WithArgs(testSpecQueue, "bad"). WillReturnError(fmt.Errorf("connection reset")) }, wantErr: true, @@ -163,26 +167,31 @@ func TestSpeculationPathSetStore_Get(t *testing.T) { func TestSpeculationPathSetStore_Create(t *testing.T) { set := testPathSet("monorepo/batch/2") + otherQueueSet := testPathSet("monorepo/batch/2") + otherQueueSet.Queue = "other-queue" tests := []struct { name string + set entity.SpeculationPathSet setup func(mock sqlmock.Sqlmock) wantErr bool wantErrIs error }{ { name: "success", + set: set, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("INSERT INTO speculation_path_set"). - WithArgs(set.Head, sqlmock.AnyArg(), set.Version). + WithArgs(set.Queue, set.Head, sqlmock.AnyArg(), set.Version). WillReturnResult(sqlmock.NewResult(0, 1)) }, }, { name: "duplicate head returns ErrAlreadyExists", + set: set, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("INSERT INTO speculation_path_set"). - WithArgs(set.Head, sqlmock.AnyArg(), set.Version). + WithArgs(set.Queue, set.Head, sqlmock.AnyArg(), set.Version). WillReturnError(&mysql.MySQLError{Number: mysqlErrDuplicateEntry}) }, wantErr: true, @@ -190,13 +199,20 @@ func TestSpeculationPathSetStore_Create(t *testing.T) { }, { name: "other exec error", + set: set, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("INSERT INTO speculation_path_set"). - WithArgs(set.Head, sqlmock.AnyArg(), set.Version). + WithArgs(set.Queue, set.Head, sqlmock.AnyArg(), set.Version). WillReturnError(fmt.Errorf("connection reset")) }, wantErr: true, }, + { + name: "queue mismatch is rejected without touching the database", + set: otherQueueSet, + setup: func(sqlmock.Sqlmock) {}, + wantErr: true, + }, } for _, tt := range tests { @@ -206,7 +222,7 @@ func TestSpeculationPathSetStore_Create(t *testing.T) { tt.setup(mock) - err := store.Create(context.Background(), set) + err := store.Create(context.Background(), tt.set) if tt.wantErr { require.Error(t, err) if tt.wantErrIs != nil { @@ -223,26 +239,31 @@ func TestSpeculationPathSetStore_Create(t *testing.T) { func TestSpeculationPathSetStore_Update(t *testing.T) { const oldVersion, newVersion = int32(3), int32(4) set := testPathSet("monorepo/batch/2") + otherQueueSet := testPathSet("monorepo/batch/2") + otherQueueSet.Queue = "other-queue" tests := []struct { name string + set entity.SpeculationPathSet setup func(mock sqlmock.Sqlmock) wantErr bool wantErrIs error }{ { name: "success", + set: set, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE speculation_path_set"). - WithArgs(sqlmock.AnyArg(), newVersion, set.Head, oldVersion). + WithArgs(sqlmock.AnyArg(), newVersion, set.Queue, set.Head, oldVersion). WillReturnResult(sqlmock.NewResult(0, 1)) }, }, { name: "version mismatch", + set: set, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE speculation_path_set"). - WithArgs(sqlmock.AnyArg(), newVersion, set.Head, oldVersion). + WithArgs(sqlmock.AnyArg(), newVersion, set.Queue, set.Head, oldVersion). WillReturnResult(sqlmock.NewResult(0, 0)) }, wantErr: true, @@ -250,22 +271,30 @@ func TestSpeculationPathSetStore_Update(t *testing.T) { }, { name: "exec error", + set: set, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE speculation_path_set"). - WithArgs(sqlmock.AnyArg(), newVersion, set.Head, oldVersion). + WithArgs(sqlmock.AnyArg(), newVersion, set.Queue, set.Head, oldVersion). WillReturnError(fmt.Errorf("connection reset")) }, wantErr: true, }, { name: "rows affected error", + set: set, setup: func(mock sqlmock.Sqlmock) { mock.ExpectExec("UPDATE speculation_path_set"). - WithArgs(sqlmock.AnyArg(), newVersion, set.Head, oldVersion). + WithArgs(sqlmock.AnyArg(), newVersion, set.Queue, set.Head, oldVersion). WillReturnResult(sqlmock.NewErrorResult(fmt.Errorf("driver error"))) }, wantErr: true, }, + { + name: "queue mismatch is rejected without touching the database", + set: otherQueueSet, + setup: func(sqlmock.Sqlmock) {}, + wantErr: true, + }, } for _, tt := range tests { @@ -275,7 +304,7 @@ func TestSpeculationPathSetStore_Update(t *testing.T) { tt.setup(mock) - err := store.Update(context.Background(), set, oldVersion, newVersion) + err := store.Update(context.Background(), tt.set, oldVersion, newVersion) if tt.wantErr { require.Error(t, err) if tt.wantErrIs != nil { @@ -302,7 +331,7 @@ func TestSpeculationPathSetStore_UpdateIgnoresEntityVersion(t *testing.T) { const oldVersion, newVersion = int32(3), int32(4) mock.ExpectExec("UPDATE speculation_path_set"). - WithArgs(sqlmock.AnyArg(), newVersion, set.Head, oldVersion). + WithArgs(sqlmock.AnyArg(), newVersion, set.Queue, set.Head, oldVersion). WillReturnResult(sqlmock.NewResult(0, 1)) require.NoError(t, store.Update(context.Background(), set, oldVersion, newVersion)) diff --git a/submitqueue/extension/storage/mysql/storage.go b/submitqueue/extension/storage/mysql/storage.go index f698990d7..6372993c5 100644 --- a/submitqueue/extension/storage/mysql/storage.go +++ b/submitqueue/extension/storage/mysql/storage.go @@ -29,27 +29,17 @@ import ( const mysqlErrDuplicateEntry = 1062 // Storage is the MySQL storage backend. It owns the shared connection pool and -// the global read-model stores, and binds queue-scoped store aggregates over -// the shared tables on demand via For. The wiring layer adapts For into the -// storage.Factory seam; per-queue backend routing stays a host decision. +// binds queue-scoped store aggregates over the shared tables on demand via For. +// The wiring layer adapts For into the storage.Factory seam; per-queue backend +// routing stays a host decision. type Storage struct { db *sql.DB scope tally.Scope - - requestLogStore storage.RequestLogStore - requestSummaryStore storage.RequestSummaryStore - requestURIStore storage.RequestURIStore } // NewStorage creates a new MySQL storage backend over the given connection pool. func NewStorage(db *sql.DB, scope tally.Scope) (*Storage, error) { - return &Storage{ - db: db, - scope: scope, - requestLogStore: NewRequestLogStore(db, scope.SubScope("request_log_store")), - requestSummaryStore: NewRequestSummaryStore(db, scope.SubScope("request_summary_store")), - requestURIStore: NewRequestURIStore(db, scope.SubScope("request_uri_store")), - }, nil + return &Storage{db: db, scope: scope}, nil } // For returns the queue-scoped store aggregate bound to queueName over the @@ -67,26 +57,14 @@ func (s *Storage) For(queueName string) (storage.Storage, error) { batchDependentStore: NewBatchDependentStore(s.db, s.scope.SubScope("batch_dependent_store"), queueName), queueBatchStateStore: NewQueueBatchStateStore(s.db, s.scope.SubScope("queue_batch_state_store"), queueName), buildStore: NewBuildStore(s.db, s.scope.SubScope("build_store"), queueName), - speculationPathSetStore: NewSpeculationPathSetStore(s.db, s.scope.SubScope("speculation_path_set_store")), + speculationPathSetStore: NewSpeculationPathSetStore(s.db, s.scope.SubScope("speculation_path_set_store"), queueName), requestQueueStore: NewRequestQueueSummaryStore(s.db, s.scope.SubScope("request_queue_summary_store"), queueName), + requestSummaryStore: NewRequestSummaryStore(s.db, s.scope.SubScope("request_summary_store"), queueName), + requestLogStore: NewRequestLogStore(s.db, s.scope.SubScope("request_log_store"), queueName), + requestURIStore: NewRequestURIStore(s.db, s.scope.SubScope("request_uri_store"), queueName), }, nil } -// GetRequestLogStore returns the global MySQL-backed RequestLogStore. -func (s *Storage) GetRequestLogStore() storage.RequestLogStore { - return s.requestLogStore -} - -// GetRequestSummaryStore returns the global MySQL-backed RequestSummaryStore. -func (s *Storage) GetRequestSummaryStore() storage.RequestSummaryStore { - return s.requestSummaryStore -} - -// GetRequestURIStore returns the global MySQL-backed RequestURIStore. -func (s *Storage) GetRequestURIStore() storage.RequestURIStore { - return s.requestURIStore -} - // Close closes the underlying database connection. func (s *Storage) Close() error { return s.db.Close() @@ -103,6 +81,9 @@ type boundStorage struct { buildStore storage.BuildStore speculationPathSetStore storage.SpeculationPathSetStore requestQueueStore storage.RequestQueueSummaryStore + requestSummaryStore storage.RequestSummaryStore + requestLogStore storage.RequestLogStore + requestURIStore storage.RequestURIStore } // Verify boundStorage implements the queue-scoped aggregate at compile time. @@ -152,3 +133,18 @@ func (f *boundStorage) GetSpeculationPathSetStore() storage.SpeculationPathSetSt func (f *boundStorage) GetRequestQueueSummaryStore() storage.RequestQueueSummaryStore { return f.requestQueueStore } + +// GetRequestSummaryStore returns the bound MySQL-backed RequestSummaryStore. +func (f *boundStorage) GetRequestSummaryStore() storage.RequestSummaryStore { + return f.requestSummaryStore +} + +// GetRequestLogStore returns the bound MySQL-backed RequestLogStore. +func (f *boundStorage) GetRequestLogStore() storage.RequestLogStore { + return f.requestLogStore +} + +// GetRequestURIStore returns the bound MySQL-backed RequestURIStore. +func (f *boundStorage) GetRequestURIStore() storage.RequestURIStore { + return f.requestURIStore +} diff --git a/submitqueue/extension/storage/mysql/storage_test.go b/submitqueue/extension/storage/mysql/storage_test.go index bd29d3944..75d772913 100644 --- a/submitqueue/extension/storage/mysql/storage_test.go +++ b/submitqueue/extension/storage/mysql/storage_test.go @@ -36,10 +36,6 @@ func TestNewStorage(t *testing.T) { s, err := NewStorage(db, testMetrics()) require.NoError(t, err) - assert.NotNil(t, s.GetRequestLogStore()) - assert.NotNil(t, s.GetRequestSummaryStore()) - assert.NotNil(t, s.GetRequestURIStore()) - bound, err := s.For("monorepo") require.NoError(t, err) assert.NotNil(t, bound.GetRequestStore()) @@ -49,7 +45,11 @@ func TestNewStorage(t *testing.T) { assert.NotNil(t, bound.GetBatchDependentStore()) assert.NotNil(t, bound.GetQueueBatchStateStore()) assert.NotNil(t, bound.GetBuildStore()) + assert.NotNil(t, bound.GetSpeculationPathSetStore()) assert.NotNil(t, bound.GetRequestQueueSummaryStore()) + assert.NotNil(t, bound.GetRequestSummaryStore()) + assert.NotNil(t, bound.GetRequestLogStore()) + assert.NotNil(t, bound.GetRequestURIStore()) _, err = s.For("") assert.Error(t, err, "resolving an empty queue name must fail") diff --git a/submitqueue/extension/storage/speculation_path_set_store.go b/submitqueue/extension/storage/speculation_path_set_store.go index 7746cb2c2..9138dbe11 100644 --- a/submitqueue/extension/storage/speculation_path_set_store.go +++ b/submitqueue/extension/storage/speculation_path_set_store.go @@ -24,11 +24,11 @@ import ( // SpeculationPathSetStore persists one head batch's chosen speculation paths. // -// A set is keyed by its head batch ID and versioned as a whole: every path in -// it shares that head, and the set is the unit of both replacement and -// optimistic locking. There is no lookup by anything but the head — callers -// that need a queue's live sets enumerate the heads from the batch listing they -// already hold and read each set by key. +// A set is keyed by its head batch ID within the bound queue and versioned as a +// whole: every path in it shares that head, and the set is the unit of both +// replacement and optimistic locking. There is no lookup by anything but the +// head — callers that need a queue's live sets enumerate the heads from the +// batch listing they already hold and read each set by key. type SpeculationPathSetStore interface { // Get retrieves a head's path set, where head is the head batch's ID. // Returns ErrNotFound if the head has no set yet, which is the normal state diff --git a/submitqueue/extension/storage/storage.go b/submitqueue/extension/storage/storage.go index 6bcbb629f..08e8c88d9 100644 --- a/submitqueue/extension/storage/storage.go +++ b/submitqueue/extension/storage/storage.go @@ -67,11 +67,6 @@ type Factory interface { // dependency. An instance is resolved per queue through Factory and is bound // to that queue: entity arguments whose Queue field disagrees with the // binding are rejected, and reads never surface another queue's records. -// -// The cross-queue read-model stores (RequestLogStore, RequestSummaryStore, -// RequestURIStore) are deliberately not part of this aggregate: their lookups -// start from identifiers that arrive without queue context, so they are -// injected individually as global seams. type Storage interface { // GetRequestStore returns the RequestStore instance. GetRequestStore() RequestStore @@ -99,4 +94,13 @@ type Storage interface { // GetRequestQueueSummaryStore returns the RequestQueueSummaryStore instance. GetRequestQueueSummaryStore() RequestQueueSummaryStore + + // GetRequestSummaryStore returns the RequestSummaryStore instance. + GetRequestSummaryStore() RequestSummaryStore + + // GetRequestLogStore returns the RequestLogStore instance. + GetRequestLogStore() RequestLogStore + + // GetRequestURIStore returns the RequestURIStore instance. + GetRequestURIStore() RequestURIStore } diff --git a/submitqueue/gateway/controller/cancel.go b/submitqueue/gateway/controller/cancel.go index ba4589f77..2c80ef7f7 100644 --- a/submitqueue/gateway/controller/cancel.go +++ b/submitqueue/gateway/controller/cancel.go @@ -42,23 +42,23 @@ type CancelController interface { var _ CancelController = (*cancelController)(nil) type cancelController struct { - logger *zap.SugaredLogger - metricsScope tally.Scope - requestSummaryStore storage.RequestSummaryStore - materializer *requestcore.Materializer - registry consumer.TopicRegistry + logger *zap.SugaredLogger + metricsScope tally.Scope + stores storage.Factory + materializer *requestcore.Materializer + registry consumer.TopicRegistry } // NewCancelController creates a new instance of the gateway cancel controller. // The controller writes a RequestStatusCancelling log entry through the shared materializer and // publishes cancel requests to the topic registered under topickey.TopicKeyCancel. -func NewCancelController(logger *zap.SugaredLogger, scope tally.Scope, summaries storage.RequestSummaryStore, materializer *requestcore.Materializer, registry consumer.TopicRegistry) CancelController { +func NewCancelController(logger *zap.SugaredLogger, scope tally.Scope, stores storage.Factory, materializer *requestcore.Materializer, registry consumer.TopicRegistry) CancelController { return &cancelController{ - logger: logger, - metricsScope: scope, - requestSummaryStore: summaries, - materializer: materializer, - registry: registry, + logger: logger, + metricsScope: scope, + stores: stores, + materializer: materializer, + registry: registry, } } @@ -79,15 +79,23 @@ func (c *cancelController) Cancel(ctx context.Context, req entity.CancelRequest) if req.ID == "" { return fmt.Errorf("requires the request to have a sqid specified: %w", ErrInvalidRequest) } + if err := validateQueueIdentifier(req.Queue); err != nil { + return fmt.Errorf("Cancel invalid queue: %w", err) + } c.logger.Debugw("cancel request received", "sqid", req.ID, + "queue", req.Queue, "reason", req.Reason, ) - // Verify the sqid exists before recording intent or publishing. - summary, err := c.requestSummaryStore.Get(ctx, req.ID) + // Verify the sqid exists before recording intent or publishing. The lookup is scoped + // to the caller's queue, so a sqid from another queue is simply not found. + stores, err := c.stores.For(storage.Config{QueueName: req.Queue}) if err != nil { + return fmt.Errorf("failed to resolve storage for queue %q: %w", req.Queue, err) + } + if _, err := stores.GetRequestSummaryStore().Get(ctx, req.ID); err != nil { if storage.IsNotFound(err) { metrics.NamedCounter(c.metricsScope, opName, "not_found", 1) return errs.NewUserError(&RequestNotFoundError{Sqid: req.ID}) @@ -95,10 +103,6 @@ func (c *cancelController) Cancel(ctx context.Context, req entity.CancelRequest) return fmt.Errorf("failed to look up request summary for sqid=%s: %w", req.ID, err) } - // Stamp the authoritative queue from the stored summary onto the payload, - // overriding whatever the caller supplied. - req.Queue = summary.Queue - // Record the user's intent in the request log before publishing. Writing direct to the // store (rather than via the log topic) keeps the gateway-emitted entry consistent with // the Land "accepted" entry and guarantees the entry is visible the moment Cancel returns. @@ -106,7 +110,7 @@ func (c *cancelController) Cancel(ctx context.Context, req entity.CancelRequest) if req.Reason != "" { metadata["reason"] = req.Reason } - logEntry := entity.NewRequestLog(req.ID, entity.RequestStatusCancelling, 0, "", metadata) + logEntry := entity.NewRequestLog(req.Queue, req.ID, entity.RequestStatusCancelling, 0, "", metadata) if err := c.materializer.PersistLog(ctx, logEntry); err != nil { return fmt.Errorf("failed to insert cancelling log for sqid=%s: %w", req.ID, err) } diff --git a/submitqueue/gateway/controller/cancel_test.go b/submitqueue/gateway/controller/cancel_test.go index 1e2328ba3..45cefe752 100644 --- a/submitqueue/gateway/controller/cancel_test.go +++ b/submitqueue/gateway/controller/cancel_test.go @@ -38,7 +38,7 @@ import ( // newTestCancelController builds a cancel controller over the fixture's // summary store and materializer. func newTestCancelController(ctrl *gomock.Controller, scope tally.Scope, fixture *controllerStorageFixture, registry consumer.TopicRegistry) CancelController { - return NewCancelController(zap.NewNop().Sugar(), scope, fixture.summaryStore, fixture.newMaterializer(ctrl), registry) + return NewCancelController(zap.NewNop().Sugar(), scope, fixture.newFactory(ctrl), fixture.newMaterializer(ctrl), registry) } func newCancelTestRegistry(t *testing.T, ctrl *gomock.Controller) (consumer.TopicRegistry, *queuemock.MockPublisher) { @@ -75,8 +75,8 @@ func newCancelStorageFixture(ctrl *gomock.Controller, requestID string) *control } // testCancelRequest returns a valid entity.CancelRequest for testing. -func testCancelRequest(sqid string, reason string) entity.CancelRequest { - return entity.CancelRequest{ID: sqid, Reason: reason} +func testCancelRequest(queue string, sqid string, reason string) entity.CancelRequest { + return entity.CancelRequest{ID: sqid, Queue: queue, Reason: reason} } func TestNewCancelController(t *testing.T) { @@ -93,7 +93,7 @@ func TestCancel_HappyPath(t *testing.T) { controller := newTestCancelController(ctrl, scope, newCancelStorageFixture(ctrl, "test-queue/42"), newCancelTestRegistryWithNoopPublisher(t, ctrl)) ctx := context.Background() - err := controller.Cancel(ctx, testCancelRequest("test-queue/42", "user changed their mind")) + err := controller.Cancel(ctx, testCancelRequest("test-queue", "test-queue/42", "user changed their mind")) require.NoError(t, err) @@ -124,7 +124,7 @@ func TestCancel_ReturnsErrorOnEmptySqid(t *testing.T) { controller := newTestCancelController(ctrl, tally.NoopScope, newCancelStorageFixture(ctrl, "test-queue/42"), newCancelTestRegistryWithNoopPublisher(t, ctrl)) ctx := context.Background() - err := controller.Cancel(ctx, testCancelRequest("", "anything")) + err := controller.Cancel(ctx, testCancelRequest("test-queue", "", "anything")) require.Error(t, err) assert.True(t, IsInvalidRequest(err)) @@ -148,7 +148,7 @@ func TestCancel_PublishesToQueue(t *testing.T) { controller := newTestCancelController(ctrl, tally.NoopScope, newCancelStorageFixture(ctrl, "my-queue/7"), registry) ctx := context.Background() - err := controller.Cancel(ctx, testCancelRequest("my-queue/7", "obsolete change")) + err := controller.Cancel(ctx, testCancelRequest("my-queue", "my-queue/7", "obsolete change")) require.NoError(t, err) assert.Equal(t, "cancel", publishedTopic) @@ -181,7 +181,7 @@ func TestCancel_InsertsCancellingLog(t *testing.T) { controller := newTestCancelController(ctrl, tally.NoopScope, fixture, registry) - err := controller.Cancel(context.Background(), testCancelRequest("my-queue/42", "obsolete change")) + err := controller.Cancel(context.Background(), testCancelRequest("my-queue", "my-queue/42", "obsolete change")) require.NoError(t, err) fixture.mu.Lock() @@ -205,7 +205,7 @@ func TestCancel_LogInsertFailure(t *testing.T) { _ = publisher controller := newTestCancelController(ctrl, tally.NoopScope, fixture, registry) - err := controller.Cancel(context.Background(), testCancelRequest("q/1", "")) + err := controller.Cancel(context.Background(), testCancelRequest("q", "q/1", "")) require.Error(t, err) } @@ -218,7 +218,7 @@ func TestCancel_ReturnsErrorOnPublishFailure(t *testing.T) { controller := newTestCancelController(ctrl, tally.NoopScope, newCancelStorageFixture(ctrl, "test-queue/1"), registry) ctx := context.Background() - err := controller.Cancel(ctx, testCancelRequest("test-queue/1", "")) + err := controller.Cancel(ctx, testCancelRequest("test-queue", "test-queue/1", "")) require.Error(t, err) } @@ -231,7 +231,7 @@ func TestCancel_UnknownSqidIsUserError(t *testing.T) { _ = publisher controller := newTestCancelController(ctrl, tally.NoopScope, fixture, registry) - err := controller.Cancel(context.Background(), testCancelRequest("ghost/1", "")) + err := controller.Cancel(context.Background(), testCancelRequest("ghost", "ghost/1", "")) require.Error(t, err) assert.True(t, IsRequestNotFound(err)) assert.True(t, errs.IsUserError(err)) @@ -254,8 +254,8 @@ func TestCancel_RequestSummaryLookupFailure(t *testing.T) { registry, publisher := newCancelTestRegistry(t, ctrl) _ = publisher - controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, summaryStore, newControllerStorageFixture(ctrl).newMaterializer(ctrl), registry) - err := controller.Cancel(context.Background(), testCancelRequest("q/1", "")) + controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, factoryForStorage(ctrl, storageWithSummaryStore(ctrl, summaryStore)), newControllerStorageFixture(ctrl).newMaterializer(ctrl), registry) + err := controller.Cancel(context.Background(), testCancelRequest("q", "q/1", "")) require.Error(t, err) assert.False(t, errs.IsUserError(err)) assert.False(t, IsRequestNotFound(err)) diff --git a/submitqueue/gateway/controller/land.go b/submitqueue/gateway/controller/land.go index a736e1de2..588376d44 100644 --- a/submitqueue/gateway/controller/land.go +++ b/submitqueue/gateway/controller/land.go @@ -61,6 +61,11 @@ func IsUnrecognizedQueue(err error) bool { return errors.As(err, &target) } +// counterDomainRequest names the per-queue sequence that mints request IDs. The +// sqid is built independently as "/", so the domain is a +// sequence name only and never appears in the ID. +const counterDomainRequest = "request" + // LandController handles land business logic for the gateway type LandController interface { Land(ctx context.Context, req entity.LandRequest) (entity.LandResult, error) @@ -71,8 +76,8 @@ var _ LandController = (*landController)(nil) type landController struct { logger *zap.SugaredLogger metricsScope tally.Scope - counter counter.Counter - summaries storage.RequestSummaryStore + counters counter.Factory + stores storage.Factory materializer *requestcore.Materializer queueConfigs queueconfig.Store registry consumer.TopicRegistry @@ -81,12 +86,12 @@ type landController struct { // NewLandController creates a new instance of the gateway land controller. // The controller publishes land requests to the topic registered under // topickey.TopicKeyStart in the registry. -func NewLandController(logger *zap.SugaredLogger, scope tally.Scope, counter counter.Counter, summaries storage.RequestSummaryStore, materializer *requestcore.Materializer, queueConfigs queueconfig.Store, registry consumer.TopicRegistry) LandController { +func NewLandController(logger *zap.SugaredLogger, scope tally.Scope, counters counter.Factory, stores storage.Factory, materializer *requestcore.Materializer, queueConfigs queueconfig.Store, registry consumer.TopicRegistry) LandController { return &landController{ logger: logger, metricsScope: scope.SubScope("land_controller"), - counter: counter, - summaries: summaries, + counters: counters, + stores: stores, materializer: materializer, queueConfigs: queueConfigs, registry: registry, @@ -118,7 +123,15 @@ func (c *landController) Land(ctx context.Context, req entity.LandRequest) (resu // Generate a globally unique request ID for the land request. // The inbound entity arrives with an empty ID; the controller owns minting it. - seq, err := c.counter.Next(ctx, "request/"+queue) + stores, err := c.stores.For(storage.Config{QueueName: queue}) + if err != nil { + return entity.LandResult{}, fmt.Errorf("failed to resolve storage for queue=%s: %w", queue, err) + } + queueCounter, err := c.counters.For(counter.Config{QueueName: queue}) + if err != nil { + return entity.LandResult{}, fmt.Errorf("failed to resolve counter for queue=%s: %w", queue, err) + } + seq, err := queueCounter.Next(ctx, counterDomainRequest) if err != nil { return entity.LandResult{}, fmt.Errorf("failed to generate request ID for queue=%s: %w", queue, err) } @@ -138,7 +151,7 @@ func (c *landController) Land(ctx context.Context, req entity.LandRequest) (resu Version: 1, Metadata: map[string]string{}, } - if err := c.summaries.Create(ctx, summary); err != nil { + if err := stores.GetRequestSummaryStore().Create(ctx, summary); err != nil { return entity.LandResult{}, fmt.Errorf("failed to create request receipt sqid=%s: %w", req.ID, err) } @@ -150,6 +163,7 @@ func (c *landController) Land(ctx context.Context, req entity.LandRequest) (resu logEntry := entity.RequestLog{ RequestID: req.ID, + Queue: req.Queue, TimestampMs: receivedAtMs, Status: entity.RequestStatusAccepted, Metadata: map[string]string{}, diff --git a/submitqueue/gateway/controller/land_test.go b/submitqueue/gateway/controller/land_test.go index 53ddd59dd..de7b22249 100644 --- a/submitqueue/gateway/controller/land_test.go +++ b/submitqueue/gateway/controller/land_test.go @@ -67,10 +67,26 @@ func newTestRegistryWithNoopPublisher(t *testing.T, ctrl *gomock.Controller) con return registry } +// staticCounterFactory resolves every queue to the same counter, so tests can keep +// setting expectations on one mock regardless of which queue the controller resolves. +// It records the queue it was asked for. +type staticCounterFactory struct { + counter counter.Counter + // forQueue is the queue named in the most recent For call. + forQueue *string +} + +func (f staticCounterFactory) For(config counter.Config) (counter.Counter, error) { + if f.forQueue != nil { + *f.forQueue = config.QueueName + } + return f.counter, nil +} + // noopStorage returns stateful request storage whose writes succeed. func newNoopLandController(t *testing.T, ctrl *gomock.Controller, cnt counter.Counter) LandController { fixture := newControllerStorageFixture(ctrl) - return NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, fixture.summaryStore, fixture.newMaterializer(ctrl), noopQueueConfigStore(ctrl), newTestRegistryWithNoopPublisher(t, ctrl)) + return NewLandController(zap.NewNop().Sugar(), tally.NoopScope, staticCounterFactory{counter: cnt}, fixture.newFactory(ctrl), fixture.newMaterializer(ctrl), noopQueueConfigStore(ctrl), newTestRegistryWithNoopPublisher(t, ctrl)) } // noopQueueConfigStore returns a mock queueconfig.Store that always reports @@ -126,8 +142,11 @@ func TestLand_ReturnsErrorOnCounterFailure(t *testing.T) { require.Error(t, err) } -func TestLand_CounterDomainIncludesQueue(t *testing.T) { - var capturedDomain string +// TestLand_ResolvesCounterForRequestQueue pins that the queue reaches the counter +// through the factory binding rather than through the domain string: the domain is a +// bare sequence name, and the sqid is still "/". +func TestLand_ResolvesCounterForRequestQueue(t *testing.T) { + var capturedDomain, capturedQueue string ctrl := gomock.NewController(t) @@ -138,13 +157,24 @@ func TestLand_CounterDomainIncludesQueue(t *testing.T) { return 1, nil }, ) - controller := newNoopLandController(t, ctrl, cnt) + fixture := newControllerStorageFixture(ctrl) + controller := NewLandController( + zap.NewNop().Sugar(), + tally.NoopScope, + staticCounterFactory{counter: cnt, forQueue: &capturedQueue}, + fixture.newFactory(ctrl), + fixture.newMaterializer(ctrl), + noopQueueConfigStore(ctrl), + newTestRegistryWithNoopPublisher(t, ctrl), + ) ctx := context.Background() - _, err := controller.Land(ctx, testLandRequest("my-queue")) + result, err := controller.Land(ctx, testLandRequest("my-queue")) require.NoError(t, err) - assert.Equal(t, "request/my-queue", capturedDomain) + assert.Equal(t, "request", capturedDomain, "the domain is a sequence name, not a queue-qualified key") + assert.Equal(t, "my-queue", capturedQueue, "the queue reaches the counter through the factory binding") + assert.Equal(t, "my-queue/1", result.ID) } func TestLand_ReturnsErrorOnEmptyQueue(t *testing.T) { @@ -264,7 +294,7 @@ func TestLand_ReturnsUnrecognizedQueueWhenStoreReportsNotFound(t *testing.T) { qcs.EXPECT().Get(gomock.Any(), "missing-queue").Return(entity.QueueConfig{}, queueconfig.ErrNotFound) fixture := newControllerStorageFixture(ctrl) - controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, fixture.summaryStore, fixture.newMaterializer(ctrl), qcs, newTestRegistryWithNoopPublisher(t, ctrl)) + controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, staticCounterFactory{counter: cnt}, fixture.newFactory(ctrl), fixture.newMaterializer(ctrl), qcs, newTestRegistryWithNoopPublisher(t, ctrl)) ctx := context.Background() _, err := controller.Land(ctx, testLandRequest("missing-queue")) @@ -287,7 +317,7 @@ func TestLand_PropagatesQueueConfigStoreError(t *testing.T) { qcs.EXPECT().Get(gomock.Any(), "test-queue").Return(entity.QueueConfig{}, fmt.Errorf("config backend down")) fixture := newControllerStorageFixture(ctrl) - controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, fixture.summaryStore, fixture.newMaterializer(ctrl), qcs, newTestRegistryWithNoopPublisher(t, ctrl)) + controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, staticCounterFactory{counter: cnt}, fixture.newFactory(ctrl), fixture.newMaterializer(ctrl), qcs, newTestRegistryWithNoopPublisher(t, ctrl)) ctx := context.Background() _, err := controller.Land(ctx, testLandRequest("test-queue")) @@ -317,9 +347,12 @@ func TestLand_PublishesToQueue(t *testing.T) { queueStore := storagemock.NewMockRequestQueueSummaryStore(ctrl) logStore := storagemock.NewMockRequestLogStore(ctrl) store.EXPECT().GetRequestQueueSummaryStore().Return(queueStore).AnyTimes() + store.EXPECT().GetRequestSummaryStore().Return(summaryStore).AnyTimes() + store.EXPECT().GetRequestLogStore().Return(logStore).AnyTimes() + store.EXPECT().GetRequestURIStore().Return(uriStore).AnyTimes() factory := storagemock.NewMockFactory(ctrl) factory.EXPECT().For(gomock.Any()).Return(store, nil).AnyTimes() - materializer := requestcore.NewMaterializer(logStore, summaryStore, uriStore, factory) + materializer := requestcore.NewMaterializer(factory) registry, publisher := newTestRegistry(t, ctrl) gomock.InOrder( @@ -373,7 +406,7 @@ func TestLand_PublishesToQueue(t *testing.T) { ), ) - controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, summaryStore, materializer, noopQueueConfigStore(ctrl), registry) + controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, staticCounterFactory{counter: cnt}, factoryForStorage(ctrl, store), materializer, noopQueueConfigStore(ctrl), registry) ctx := context.Background() req := entity.LandRequest{ @@ -399,6 +432,7 @@ func TestLand_PublishesToQueue(t *testing.T) { assert.Positive(t, receiptSummary.ReceivedAtMs) assert.Equal(t, entity.RequestLog{ RequestID: "test-queue/123", + Queue: "test-queue", TimestampMs: receiptSummary.ReceivedAtMs, Status: entity.RequestStatusAccepted, Metadata: map[string]string{}, @@ -407,6 +441,7 @@ func TestLand_PublishesToQueue(t *testing.T) { assert.Equal(t, int32(2), materializedSummary.Version) assert.Equal(t, entity.RequestURI{ ChangeURI: "github://github.example.com/uber/backend/pull/456/fedcba9876543210fedcba9876543210fedcba98", + Queue: "test-queue", ReceivedAtMs: receiptSummary.ReceivedAtMs, RequestID: "test-queue/123", }, persistedMapping) @@ -449,7 +484,7 @@ func TestLand_ReturnsErrorWhenPublishFails(t *testing.T) { registry, publisher := newTestRegistry(t, ctrl) publisher.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("queue unavailable")) - controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, summaryStore, newControllerStorageFixture(ctrl).newMaterializer(ctrl), noopQueueConfigStore(ctrl), registry) + controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, staticCounterFactory{counter: cnt}, factoryForStorage(ctrl, storageWithSummaryStore(ctrl, summaryStore)), newControllerStorageFixture(ctrl).newMaterializer(ctrl), noopQueueConfigStore(ctrl), registry) ctx := context.Background() _, err := controller.Land(ctx, testLandRequest("test-queue")) @@ -467,8 +502,8 @@ func TestLand_ReturnsSqidWhenAcceptedLogFailsAfterPublish(t *testing.T) { controller := NewLandController( zap.NewNop().Sugar(), tally.NoopScope, - cnt, - fixture.summaryStore, + staticCounterFactory{counter: cnt}, + fixture.newFactory(ctrl), fixture.newMaterializer(ctrl), noopQueueConfigStore(ctrl), newTestRegistryWithNoopPublisher(t, ctrl), diff --git a/submitqueue/gateway/controller/log/log_test.go b/submitqueue/gateway/controller/log/log_test.go index 47632dd48..0cf73debc 100644 --- a/submitqueue/gateway/controller/log/log_test.go +++ b/submitqueue/gateway/controller/log/log_test.go @@ -34,13 +34,7 @@ import ( // newUnusedMaterializer returns a materializer whose stores expect no calls, // for cases that fail before any persistence. func newUnusedMaterializer(ctrl *gomock.Controller) *requestcore.Materializer { - factory := storagemock.NewMockFactory(ctrl) - return requestcore.NewMaterializer( - storagemock.NewMockRequestLogStore(ctrl), - storagemock.NewMockRequestSummaryStore(ctrl), - storagemock.NewMockRequestURIStore(ctrl), - factory, - ) + return requestcore.NewMaterializer(storagemock.NewMockFactory(ctrl)) } func TestController_Process(t *testing.T) { @@ -130,9 +124,12 @@ func newLogControllerStore(ctrl *gomock.Controller, insertErr, getErr, updateErr queueStore := storagemock.NewMockRequestQueueSummaryStore(ctrl) uriStore := storagemock.NewMockRequestURIStore(ctrl) store.EXPECT().GetRequestQueueSummaryStore().Return(queueStore).AnyTimes() + store.EXPECT().GetRequestSummaryStore().Return(summaryStore).AnyTimes() + store.EXPECT().GetRequestLogStore().Return(logStore).AnyTimes() + store.EXPECT().GetRequestURIStore().Return(uriStore).AnyTimes() factory := storagemock.NewMockFactory(ctrl) factory.EXPECT().For(gomock.Any()).Return(store, nil).AnyTimes() - materializer := requestcore.NewMaterializer(logStore, summaryStore, uriStore, factory) + materializer := requestcore.NewMaterializer(factory) logStore.EXPECT().Insert(gomock.Any(), gomock.Any()).Return(insertErr) if insertErr != nil { return materializer @@ -157,6 +154,6 @@ func newLogControllerStore(ctrl *gomock.Controller, insertErr, getErr, updateErr } func newRequestLog(requestID string, status entity.RequestStatus, requestVersion int32, lastError string, metadata map[string]string) *entity.RequestLog { - log := entity.NewRequestLog(requestID, status, requestVersion, lastError, metadata) + log := entity.NewRequestLog("test-queue", requestID, status, requestVersion, lastError, metadata) return &log } diff --git a/submitqueue/gateway/controller/request_history.go b/submitqueue/gateway/controller/request_history.go index 1a7141ca0..9a0151d15 100644 --- a/submitqueue/gateway/controller/request_history.go +++ b/submitqueue/gateway/controller/request_history.go @@ -38,19 +38,17 @@ type RequestHistoryController interface { var _ RequestHistoryController = (*requestHistoryController)(nil) type requestHistoryController struct { - logger *zap.SugaredLogger - metricsScope tally.Scope - requestLogStore storage.RequestLogStore - requestURIStore storage.RequestURIStore + logger *zap.SugaredLogger + metricsScope tally.Scope + stores storage.Factory } // NewRequestHistoryController creates a gateway request-history controller. -func NewRequestHistoryController(logger *zap.SugaredLogger, scope tally.Scope, requestLogStore storage.RequestLogStore, requestURIStore storage.RequestURIStore) RequestHistoryController { +func NewRequestHistoryController(logger *zap.SugaredLogger, scope tally.Scope, stores storage.Factory) RequestHistoryController { return &requestHistoryController{ - logger: logger, - metricsScope: scope.SubScope("request_history_controller"), - requestLogStore: requestLogStore, - requestURIStore: requestURIStore, + logger: logger, + metricsScope: scope.SubScope("request_history_controller"), + stores: stores, } } @@ -62,8 +60,16 @@ func (c *requestHistoryController) GetRequestHistoryByID(ctx context.Context, re if err := validateStoredIdentifier("sqid", req.ID); err != nil { return nil, fmt.Errorf("GetRequestHistoryByID invalid request: %w", err) } + if err := validateQueueIdentifier(req.Queue); err != nil { + return nil, fmt.Errorf("GetRequestHistoryByID invalid queue: %w", err) + } + + stores, err := c.stores.For(storage.Config{QueueName: req.Queue}) + if err != nil { + return nil, fmt.Errorf("GetRequestHistoryByID failed to resolve storage for queue %q: %w", req.Queue, err) + } - logs, err := c.requestLogStore.List(ctx, req.ID) + logs, err = stores.GetRequestLogStore().List(ctx, req.ID) if err != nil { if storage.IsNotFound(err) { return nil, errs.NewUserError(&RequestNotFoundError{Sqid: req.ID}) @@ -86,8 +92,17 @@ func (c *requestHistoryController) GetRequestHistoryByChangeURI(ctx context.Cont if err := validateStoredIdentifier("change URI", req.ChangeURI); err != nil { return nil, fmt.Errorf("GetRequestHistoryByChangeURI invalid request: %w", err) } + if err := validateQueueIdentifier(req.Queue); err != nil { + return nil, fmt.Errorf("GetRequestHistoryByChangeURI invalid queue: %w", err) + } + + stores, err := c.stores.For(storage.Config{QueueName: req.Queue}) + if err != nil { + return nil, fmt.Errorf("GetRequestHistoryByChangeURI failed to resolve storage for queue %q: %w", req.Queue, err) + } + logStore := stores.GetRequestLogStore() - mappings, err := c.requestURIStore.ListByURI(ctx, req.ChangeURI, maxChangeRequestResults+1) + mappings, err := stores.GetRequestURIStore().ListByURI(ctx, req.ChangeURI, maxChangeRequestResults+1) if err != nil { return nil, fmt.Errorf("GetRequestHistoryByChangeURI failed to list request mappings change_uri=%s: %w", req.ChangeURI, err) } @@ -100,7 +115,7 @@ func (c *requestHistoryController) GetRequestHistoryByChangeURI(ctx context.Cont histories := make([]requestHistoryWithCounter, 0, len(mappings)) for _, mapping := range mappings { - logs, err := c.requestLogStore.List(ctx, mapping.RequestID) + logs, err := logStore.List(ctx, mapping.RequestID) if err != nil { if storage.IsNotFound(err) { continue diff --git a/submitqueue/gateway/controller/request_history_test.go b/submitqueue/gateway/controller/request_history_test.go index 0898aa824..f6b6e38f7 100644 --- a/submitqueue/gateway/controller/request_history_test.go +++ b/submitqueue/gateway/controller/request_history_test.go @@ -40,8 +40,8 @@ func TestGetRequestHistoryByID(t *testing.T) { {RequestID: "queue/1", TimestampMs: 20, Status: entity.RequestStatusStarted, LastError: "retry", Metadata: map[string]string{"attempt": "1"}}, }, nil) - controller := NewRequestHistoryController(zap.NewNop().Sugar(), tally.NoopScope, logStore, uriStore) - events, err := controller.GetRequestHistoryByID(context.Background(), entity.GetRequestHistoryByIDRequest{ID: "queue/1"}) + controller := NewRequestHistoryController(zap.NewNop().Sugar(), tally.NoopScope, readModelFactory(ctrl, nil, logStore, uriStore)) + events, err := controller.GetRequestHistoryByID(context.Background(), entity.GetRequestHistoryByIDRequest{ID: "queue/1", Queue: "queue"}) require.NoError(t, err) require.Len(t, events, 3) @@ -68,8 +68,8 @@ func TestGetRequestHistoryByChangeURI(t *testing.T) { logStore.EXPECT().List(gomock.Any(), "queue/1").Return([]entity.RequestLog{{RequestID: "queue/1", TimestampMs: 1, Status: entity.RequestStatusAccepted}}, nil) logStore.EXPECT().List(gomock.Any(), "a/2").Return([]entity.RequestLog{{RequestID: "a/2", TimestampMs: 2, Status: entity.RequestStatusError}}, nil) - controller := NewRequestHistoryController(zap.NewNop().Sugar(), tally.NoopScope, logStore, uriStore) - histories, err := controller.GetRequestHistoryByChangeURI(context.Background(), entity.GetRequestHistoryByChangeURIRequest{ChangeURI: "uri"}) + controller := NewRequestHistoryController(zap.NewNop().Sugar(), tally.NoopScope, readModelFactory(ctrl, nil, logStore, uriStore)) + histories, err := controller.GetRequestHistoryByChangeURI(context.Background(), entity.GetRequestHistoryByChangeURIRequest{ChangeURI: "uri", Queue: "queue"}) require.NoError(t, err) require.Len(t, histories, 4) @@ -117,7 +117,7 @@ func TestHistoryErrors(t *testing.T) { logStore.EXPECT().List(gomock.Any(), "missing/1").Return(nil, storage.ErrNotFound) }, call: func(c RequestHistoryController) error { - _, err := c.GetRequestHistoryByID(context.Background(), entity.GetRequestHistoryByIDRequest{ID: "missing/1"}) + _, err := c.GetRequestHistoryByID(context.Background(), entity.GetRequestHistoryByIDRequest{ID: "missing/1", Queue: "missing"}) return err }, wantNotFound: true, @@ -129,7 +129,7 @@ func TestHistoryErrors(t *testing.T) { logStore.EXPECT().List(gomock.Any(), "queue/1").Return(nil, backendErr) }, call: func(c RequestHistoryController) error { - _, err := c.GetRequestHistoryByID(context.Background(), entity.GetRequestHistoryByIDRequest{ID: "queue/1"}) + _, err := c.GetRequestHistoryByID(context.Background(), entity.GetRequestHistoryByIDRequest{ID: "queue/1", Queue: "queue"}) return err }, }, @@ -139,7 +139,7 @@ func TestHistoryErrors(t *testing.T) { uriStore.EXPECT().ListByURI(gomock.Any(), "uri", 101).Return(nil, nil) }, call: func(c RequestHistoryController) error { - _, err := c.GetRequestHistoryByChangeURI(context.Background(), entity.GetRequestHistoryByChangeURIRequest{ChangeURI: "uri"}) + _, err := c.GetRequestHistoryByChangeURI(context.Background(), entity.GetRequestHistoryByChangeURIRequest{ChangeURI: "uri", Queue: "queue"}) return err }, wantNotFound: true, @@ -151,7 +151,7 @@ func TestHistoryErrors(t *testing.T) { uriStore.EXPECT().ListByURI(gomock.Any(), "uri", 101).Return(make([]entity.RequestURI, 101), nil) }, call: func(c RequestHistoryController) error { - _, err := c.GetRequestHistoryByChangeURI(context.Background(), entity.GetRequestHistoryByChangeURIRequest{ChangeURI: "uri"}) + _, err := c.GetRequestHistoryByChangeURI(context.Background(), entity.GetRequestHistoryByChangeURIRequest{ChangeURI: "uri", Queue: "queue"}) return err }, wantTooMany: true, @@ -164,7 +164,7 @@ func TestHistoryErrors(t *testing.T) { logStore.EXPECT().List(gomock.Any(), "queue/1").Return(nil, storage.ErrNotFound) }, call: func(c RequestHistoryController) error { - _, err := c.GetRequestHistoryByChangeURI(context.Background(), entity.GetRequestHistoryByChangeURIRequest{ChangeURI: "uri"}) + _, err := c.GetRequestHistoryByChangeURI(context.Background(), entity.GetRequestHistoryByChangeURIRequest{ChangeURI: "uri", Queue: "queue"}) return err }, wantNotFound: true, @@ -177,7 +177,7 @@ func TestHistoryErrors(t *testing.T) { logStore.EXPECT().List(gomock.Any(), "malformed").Return([]entity.RequestLog{{RequestID: "malformed"}}, nil) }, call: func(c RequestHistoryController) error { - _, err := c.GetRequestHistoryByChangeURI(context.Background(), entity.GetRequestHistoryByChangeURIRequest{ChangeURI: "uri"}) + _, err := c.GetRequestHistoryByChangeURI(context.Background(), entity.GetRequestHistoryByChangeURIRequest{ChangeURI: "uri", Queue: "queue"}) return err }, wantInternal: true, @@ -192,7 +192,7 @@ func TestHistoryErrors(t *testing.T) { if tt.setup != nil { tt.setup(logStore, uriStore) } - controller := NewRequestHistoryController(zap.NewNop().Sugar(), tally.NoopScope, logStore, uriStore) + controller := NewRequestHistoryController(zap.NewNop().Sugar(), tally.NoopScope, readModelFactory(ctrl, nil, logStore, uriStore)) err := tt.call(controller) diff --git a/submitqueue/gateway/controller/request_summary.go b/submitqueue/gateway/controller/request_summary.go index 8359a7210..aa17d8359 100644 --- a/submitqueue/gateway/controller/request_summary.go +++ b/submitqueue/gateway/controller/request_summary.go @@ -35,19 +35,17 @@ type RequestSummaryController interface { var _ RequestSummaryController = (*requestSummaryController)(nil) type requestSummaryController struct { - logger *zap.SugaredLogger - metricsScope tally.Scope - requestSummaryStore storage.RequestSummaryStore - requestURIStore storage.RequestURIStore + logger *zap.SugaredLogger + metricsScope tally.Scope + stores storage.Factory } // NewRequestSummaryController creates a gateway request-summary controller. -func NewRequestSummaryController(logger *zap.SugaredLogger, scope tally.Scope, requestSummaryStore storage.RequestSummaryStore, requestURIStore storage.RequestURIStore) RequestSummaryController { +func NewRequestSummaryController(logger *zap.SugaredLogger, scope tally.Scope, stores storage.Factory) RequestSummaryController { return &requestSummaryController{ - logger: logger, - metricsScope: scope.SubScope("request_summary_controller"), - requestSummaryStore: requestSummaryStore, - requestURIStore: requestURIStore, + logger: logger, + metricsScope: scope.SubScope("request_summary_controller"), + stores: stores, } } @@ -59,8 +57,16 @@ func (c *requestSummaryController) GetRequestSummaryByID(ctx context.Context, re if err := validateStoredIdentifier("sqid", req.ID); err != nil { return entity.RequestSummary{}, fmt.Errorf("GetRequestSummaryByID invalid request: %w", err) } + if err := validateQueueIdentifier(req.Queue); err != nil { + return entity.RequestSummary{}, fmt.Errorf("GetRequestSummaryByID invalid queue: %w", err) + } + + stores, err := c.stores.For(storage.Config{QueueName: req.Queue}) + if err != nil { + return entity.RequestSummary{}, fmt.Errorf("GetRequestSummaryByID failed to resolve storage for queue %q: %w", req.Queue, err) + } - summary, err := c.requestSummaryStore.Get(ctx, req.ID) + summary, err = stores.GetRequestSummaryStore().Get(ctx, req.ID) if err != nil { if storage.IsNotFound(err) { return entity.RequestSummary{}, errs.NewUserError(&RequestNotFoundError{Sqid: req.ID}) @@ -86,8 +92,17 @@ func (c *requestSummaryController) GetRequestSummaryByChangeURI(ctx context.Cont if err := validateStoredIdentifier("change URI", req.ChangeURI); err != nil { return nil, fmt.Errorf("GetRequestSummaryByChangeURI invalid request: %w", err) } + if err := validateQueueIdentifier(req.Queue); err != nil { + return nil, fmt.Errorf("GetRequestSummaryByChangeURI invalid queue: %w", err) + } + + stores, err := c.stores.For(storage.Config{QueueName: req.Queue}) + if err != nil { + return nil, fmt.Errorf("GetRequestSummaryByChangeURI failed to resolve storage for queue %q: %w", req.Queue, err) + } + summaryStore := stores.GetRequestSummaryStore() - mappings, err := c.requestURIStore.ListByURI(ctx, req.ChangeURI, maxChangeRequestResults+1) + mappings, err := stores.GetRequestURIStore().ListByURI(ctx, req.ChangeURI, maxChangeRequestResults+1) if err != nil { return nil, fmt.Errorf("GetRequestSummaryByChangeURI failed to list request mappings change_uri=%s: %w", req.ChangeURI, err) } @@ -100,7 +115,7 @@ func (c *requestSummaryController) GetRequestSummaryByChangeURI(ctx context.Cont requests := make([]entity.RequestSummary, 0, len(mappings)) for _, mapping := range mappings { - summary, err := c.requestSummaryStore.Get(ctx, mapping.RequestID) + summary, err := summaryStore.Get(ctx, mapping.RequestID) if err != nil { if storage.IsNotFound(err) { return nil, &InternalConsistencyError{Message: fmt.Sprintf("request summary missing for mapped change URI %q and sqid %q", req.ChangeURI, mapping.RequestID)} diff --git a/submitqueue/gateway/controller/request_summary_test.go b/submitqueue/gateway/controller/request_summary_test.go index 04440f5e1..7ee5f555c 100644 --- a/submitqueue/gateway/controller/request_summary_test.go +++ b/submitqueue/gateway/controller/request_summary_test.go @@ -44,8 +44,8 @@ func TestGetRequestSummaryByID(t *testing.T) { Metadata: map[string]string{"k": "v"}, }, nil) - controller := NewRequestSummaryController(zap.NewNop().Sugar(), tally.NoopScope, summaryStore, uriStore) - summary, err := controller.GetRequestSummaryByID(context.Background(), entity.GetRequestSummaryByIDRequest{ID: "test-queue/1"}) + controller := NewRequestSummaryController(zap.NewNop().Sugar(), tally.NoopScope, readModelFactory(ctrl, summaryStore, nil, uriStore)) + summary, err := controller.GetRequestSummaryByID(context.Background(), entity.GetRequestSummaryByIDRequest{ID: "test-queue/1", Queue: "test-queue"}) require.NoError(t, err) assert.Equal(t, "test-queue/1", summary.RequestID) @@ -68,8 +68,8 @@ func TestGetRequestSummaryByChangeURI(t *testing.T) { summaryStore.EXPECT().Get(gomock.Any(), "queue/2").Return(entity.RequestSummary{RequestID: "queue/2", ReceivedAtMs: 200, Status: entity.RequestStatusLanded, ChangeURIs: []string{}}, nil) summaryStore.EXPECT().Get(gomock.Any(), "queue/1").Return(entity.RequestSummary{RequestID: "queue/1", ReceivedAtMs: 100, Status: entity.RequestStatusError, ChangeURIs: []string{}}, nil) - controller := NewRequestSummaryController(zap.NewNop().Sugar(), tally.NoopScope, summaryStore, uriStore) - summaries, err := controller.GetRequestSummaryByChangeURI(context.Background(), entity.GetRequestSummaryByChangeURIRequest{ChangeURI: "uri"}) + controller := NewRequestSummaryController(zap.NewNop().Sugar(), tally.NoopScope, readModelFactory(ctrl, summaryStore, nil, uriStore)) + summaries, err := controller.GetRequestSummaryByChangeURI(context.Background(), entity.GetRequestSummaryByChangeURIRequest{ChangeURI: "uri", Queue: "queue"}) require.NoError(t, err) require.Len(t, summaries, 2) @@ -112,7 +112,7 @@ func TestStatusErrors(t *testing.T) { summaryStore.EXPECT().Get(gomock.Any(), "missing/1").Return(entity.RequestSummary{}, storage.ErrNotFound) }, call: func(c RequestSummaryController) error { - _, err := c.GetRequestSummaryByID(context.Background(), entity.GetRequestSummaryByIDRequest{ID: "missing/1"}) + _, err := c.GetRequestSummaryByID(context.Background(), entity.GetRequestSummaryByIDRequest{ID: "missing/1", Queue: "missing"}) return err }, wantNotFound: true, @@ -127,7 +127,7 @@ func TestStatusErrors(t *testing.T) { }, nil) }, call: func(c RequestSummaryController) error { - _, err := c.GetRequestSummaryByID(context.Background(), entity.GetRequestSummaryByIDRequest{ID: "queue/1"}) + _, err := c.GetRequestSummaryByID(context.Background(), entity.GetRequestSummaryByIDRequest{ID: "queue/1", Queue: "queue"}) return err }, wantNotFound: true, @@ -139,7 +139,7 @@ func TestStatusErrors(t *testing.T) { summaryStore.EXPECT().Get(gomock.Any(), "queue/1").Return(entity.RequestSummary{}, backendErr) }, call: func(c RequestSummaryController) error { - _, err := c.GetRequestSummaryByID(context.Background(), entity.GetRequestSummaryByIDRequest{ID: "queue/1"}) + _, err := c.GetRequestSummaryByID(context.Background(), entity.GetRequestSummaryByIDRequest{ID: "queue/1", Queue: "queue"}) return err }, }, @@ -149,7 +149,7 @@ func TestStatusErrors(t *testing.T) { uriStore.EXPECT().ListByURI(gomock.Any(), "uri", 101).Return([]entity.RequestURI{}, nil) }, call: func(c RequestSummaryController) error { - _, err := c.GetRequestSummaryByChangeURI(context.Background(), entity.GetRequestSummaryByChangeURIRequest{ChangeURI: "uri"}) + _, err := c.GetRequestSummaryByChangeURI(context.Background(), entity.GetRequestSummaryByChangeURIRequest{ChangeURI: "uri", Queue: "queue"}) return err }, wantNotFound: true, @@ -161,7 +161,7 @@ func TestStatusErrors(t *testing.T) { uriStore.EXPECT().ListByURI(gomock.Any(), "uri", 101).Return(make([]entity.RequestURI, 101), nil) }, call: func(c RequestSummaryController) error { - _, err := c.GetRequestSummaryByChangeURI(context.Background(), entity.GetRequestSummaryByChangeURIRequest{ChangeURI: "uri"}) + _, err := c.GetRequestSummaryByChangeURI(context.Background(), entity.GetRequestSummaryByChangeURIRequest{ChangeURI: "uri", Queue: "queue"}) return err }, wantTooMany: true, @@ -174,7 +174,7 @@ func TestStatusErrors(t *testing.T) { summaryStore.EXPECT().Get(gomock.Any(), "missing/1").Return(entity.RequestSummary{}, storage.ErrNotFound) }, call: func(c RequestSummaryController) error { - _, err := c.GetRequestSummaryByChangeURI(context.Background(), entity.GetRequestSummaryByChangeURIRequest{ChangeURI: "uri"}) + _, err := c.GetRequestSummaryByChangeURI(context.Background(), entity.GetRequestSummaryByChangeURIRequest{ChangeURI: "uri", Queue: "queue"}) return err }, wantInternal: true, @@ -189,7 +189,7 @@ func TestStatusErrors(t *testing.T) { if tt.setup != nil { tt.setup(summaryStore, uriStore) } - controller := NewRequestSummaryController(zap.NewNop().Sugar(), tally.NoopScope, summaryStore, uriStore) + controller := NewRequestSummaryController(zap.NewNop().Sugar(), tally.NoopScope, readModelFactory(ctrl, summaryStore, nil, uriStore)) err := tt.call(controller) diff --git a/submitqueue/gateway/controller/storage_fixture_test.go b/submitqueue/gateway/controller/storage_fixture_test.go index a1f9dcb2a..ccfa32213 100644 --- a/submitqueue/gateway/controller/storage_fixture_test.go +++ b/submitqueue/gateway/controller/storage_fixture_test.go @@ -51,6 +51,9 @@ func newControllerStorageFixture(ctrl *gomock.Controller) *controllerStorageFixt queueSummaries: make(map[string]entity.RequestQueueSummary), } fixture.storage.EXPECT().GetRequestQueueSummaryStore().Return(fixture.queueStore).AnyTimes() + fixture.storage.EXPECT().GetRequestSummaryStore().Return(fixture.summaryStore).AnyTimes() + fixture.storage.EXPECT().GetRequestLogStore().Return(fixture.logStore).AnyTimes() + fixture.storage.EXPECT().GetRequestURIStore().Return(fixture.uriStore).AnyTimes() fixture.summaryStore.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, summary entity.RequestSummary) error { fixture.mu.Lock() @@ -157,13 +160,48 @@ func queueSummaryTestKey(queue string, receivedAtMs int64, requestID string) str // newFactory returns a storage.Factory that resolves every queue to the // fixture's queue-scoped aggregate. func (f *controllerStorageFixture) newFactory(ctrl *gomock.Controller) storage.Factory { + return factoryForStorage(ctrl, f.storage) +} + +// factoryForStorage returns a storage.Factory resolving every queue to store. +func factoryForStorage(ctrl *gomock.Controller, store *storagemock.MockStorage) storage.Factory { factory := storagemock.NewMockFactory(ctrl) - factory.EXPECT().For(gomock.Any()).Return(f.storage, nil).AnyTimes() + factory.EXPECT().For(gomock.Any()).Return(store, nil).AnyTimes() return factory } +// storageWithSummaryStore returns a queue-scoped aggregate whose summary store is +// the given mock; the other read-model stores accept any call and do nothing. +func storageWithSummaryStore(ctrl *gomock.Controller, summaries *storagemock.MockRequestSummaryStore) *storagemock.MockStorage { + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetRequestSummaryStore().Return(summaries).AnyTimes() + return store +} + +// readModelFactory returns a storage.Factory resolving every queue to an aggregate +// exposing the given read-model stores. A nil store leaves its getter unstubbed, so +// an unexpected call fails the test rather than silently returning a zero value. +func readModelFactory( + ctrl *gomock.Controller, + summaries *storagemock.MockRequestSummaryStore, + logs *storagemock.MockRequestLogStore, + uris *storagemock.MockRequestURIStore, +) storage.Factory { + store := storagemock.NewMockStorage(ctrl) + if summaries != nil { + store.EXPECT().GetRequestSummaryStore().Return(summaries).AnyTimes() + } + if logs != nil { + store.EXPECT().GetRequestLogStore().Return(logs).AnyTimes() + } + if uris != nil { + store.EXPECT().GetRequestURIStore().Return(uris).AnyTimes() + } + return factoryForStorage(ctrl, store) +} + // newMaterializer builds a request read-model materializer over the fixture's -// global stores and its queue-scoped aggregate. +// queue-scoped aggregate. func (f *controllerStorageFixture) newMaterializer(ctrl *gomock.Controller) *requestcore.Materializer { - return requestcore.NewMaterializer(f.logStore, f.summaryStore, f.uriStore, f.newFactory(ctrl)) + return requestcore.NewMaterializer(f.newFactory(ctrl)) } diff --git a/submitqueue/orchestrator/controller/batch/BUILD.bazel b/submitqueue/orchestrator/controller/batch/BUILD.bazel index 048125fa3..29691b01e 100644 --- a/submitqueue/orchestrator/controller/batch/BUILD.bazel +++ b/submitqueue/orchestrator/controller/batch/BUILD.bazel @@ -31,6 +31,7 @@ go_test( "//platform/base/messagequeue:go_default_library", "//platform/consumer:go_default_library", "//platform/consumer/mock:go_default_library", + "//platform/extension/counter:go_default_library", "//platform/extension/counter/mock:go_default_library", "//platform/extension/messagequeue/mock:go_default_library", "//submitqueue/core/topickey:go_default_library", diff --git a/submitqueue/orchestrator/controller/batch/batch.go b/submitqueue/orchestrator/controller/batch/batch.go index 0985063f3..640fee04a 100644 --- a/submitqueue/orchestrator/controller/batch/batch.go +++ b/submitqueue/orchestrator/controller/batch/batch.go @@ -40,7 +40,7 @@ type Controller struct { logger *zap.SugaredLogger metricsScope tally.Scope registry consumer.TopicRegistry - counter counter.Counter + counters counter.Factory stores storage.Factory analyzers conflict.Factory topicKey consumer.TopicKey @@ -52,12 +52,17 @@ var _ consumer.Controller = (*Controller)(nil) const opName = "process" +// counterDomainBatch names the per-queue sequence that mints batch IDs. The batch +// ID is built independently as "/batch/", so the domain is a +// sequence name only and never appears in the ID. +const counterDomainBatch = "batch" + // NewController creates a new batch controller for the orchestrator. func NewController( logger *zap.SugaredLogger, scope tally.Scope, registry consumer.TopicRegistry, - counter counter.Counter, + counters counter.Factory, stores storage.Factory, analyzers conflict.Factory, topicKey consumer.TopicKey, @@ -67,7 +72,7 @@ func NewController( logger: logger.Named("batch_controller"), metricsScope: scope.SubScope("batch_controller"), registry: registry, - counter: counter, + counters: counters, stores: stores, analyzers: analyzers, topicKey: topicKey, @@ -134,7 +139,12 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // TODO: if capacity is full, wait here for other requests to accumulate to batch them together, or include a request into an existing batch if it's not too late. // Generate a globally unique batch ID. - seq, err := c.counter.Next(ctx, "batch/"+request.Queue) + queueCounter, err := c.counters.For(counter.Config{QueueName: request.Queue}) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "counter_errors", 1) + return fmt.Errorf("failed to resolve counter for queue=%s: %w", request.Queue, err) + } + seq, err := queueCounter.Next(ctx, counterDomainBatch) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "counter_errors", 1) return fmt.Errorf("failed to generate batch ID for queue=%s: %w", request.Queue, err) @@ -304,7 +314,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // so a redelivery that creates a fresh batch re-emits "batched" with a // different batch_id but is deduped to the first entry — acceptable, the // request is batched either way. - logEntry := entity.NewRequestLog(request.ID, entity.RequestStatusBatched, request.Version, "", map[string]string{ + logEntry := entity.NewRequestLog(request.Queue, request.ID, entity.RequestStatusBatched, request.Version, "", map[string]string{ "batch_id": batch.ID, }) if err := corerequest.PublishLog(ctx, c.registry, logEntry, request.ID); err != nil { diff --git a/submitqueue/orchestrator/controller/batch/batch_test.go b/submitqueue/orchestrator/controller/batch/batch_test.go index 331334229..58f337245 100644 --- a/submitqueue/orchestrator/controller/batch/batch_test.go +++ b/submitqueue/orchestrator/controller/batch/batch_test.go @@ -29,6 +29,7 @@ import ( entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" "github.com/uber/submitqueue/platform/consumer" consumermock "github.com/uber/submitqueue/platform/consumer/mock" + "github.com/uber/submitqueue/platform/extension/counter" countermock "github.com/uber/submitqueue/platform/extension/counter/mock" queuemock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" "github.com/uber/submitqueue/submitqueue/core/topickey" @@ -101,6 +102,12 @@ func storageFactoryFor(ctrl *gomock.Controller, store storage.Storage) *storagem return f } +// staticCounterFactory resolves every queue to the same counter, so tests can keep +// setting expectations on one mock regardless of which queue the controller resolves. +type staticCounterFactory struct{ counter counter.Counter } + +func (f staticCounterFactory) For(counter.Config) (counter.Counter, error) { return f.counter, nil } + // testRequest returns a standard test request for batch tests. func testRequest() entity.Request { return entity.Request{ @@ -175,7 +182,7 @@ func newTestController(t *testing.T, ctrl *gomock.Controller, cnt *countermock.M analyzerFactory := conflictmock.NewMockFactory(ctrl) analyzerFactory.EXPECT().For(gomock.Any()).Return(analyzer, nil).AnyTimes() - return NewController(logger, scope, registry, cnt, storageFactoryFor(ctrl, mockStorage), analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch") + return NewController(logger, scope, registry, staticCounterFactory{counter: cnt}, storageFactoryFor(ctrl, mockStorage), analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch") } func TestNewController(t *testing.T) { @@ -280,7 +287,7 @@ func TestController_Process_StampsQueueOnSpeculatePayload(t *testing.T) { analyzerFactory := conflictmock.NewMockFactory(ctrl) analyzerFactory.EXPECT().For(gomock.Any()).Return(all.New(), nil).AnyTimes() controller := NewController( - zaptest.NewLogger(t).Sugar(), tally.NoopScope, registry, newSequentialCounter(ctrl), + zaptest.NewLogger(t).Sugar(), tally.NoopScope, registry, staticCounterFactory{counter: newSequentialCounter(ctrl)}, storageFactoryFor(ctrl, mockStorage), analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch", ) @@ -362,7 +369,7 @@ func TestController_Process_PublishesBatchedLog(t *testing.T) { analyzerFactory := conflictmock.NewMockFactory(ctrl) analyzerFactory.EXPECT().For(gomock.Any()).Return(all.New(), nil).AnyTimes() controller := NewController( - zaptest.NewLogger(t).Sugar(), tally.NoopScope, registry, newSequentialCounter(ctrl), + zaptest.NewLogger(t).Sugar(), tally.NoopScope, registry, staticCounterFactory{counter: newSequentialCounter(ctrl)}, storageFactoryFor(ctrl, mockStorage), analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch", ) @@ -817,7 +824,7 @@ func TestController_Process_CASLostToCancel(t *testing.T) { analyzerFactory := conflictmock.NewMockFactory(ctrl) analyzerFactory.EXPECT().For(gomock.Any()).Return(all.New(), nil).AnyTimes() controller := NewController( - zaptest.NewLogger(t).Sugar(), tally.NoopScope, registry, newSequentialCounter(ctrl), + zaptest.NewLogger(t).Sugar(), tally.NoopScope, registry, staticCounterFactory{counter: newSequentialCounter(ctrl)}, storageFactoryFor(ctrl, mockStorage), analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch", ) @@ -941,7 +948,7 @@ func TestController_Process_ReadiesBatchBeforePublishing(t *testing.T) { } cnt := countermock.NewMockCounter(ctrl) - cnt.EXPECT().Next(gomock.Any(), "batch/"+request.Queue).Return(int64(7), nil) + cnt.EXPECT().Next(gomock.Any(), counterDomainBatch).Return(int64(7), nil) requestStore := storagemock.NewMockRequestStore(ctrl) requestStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) @@ -987,7 +994,7 @@ func TestController_Process_ReadiesBatchBeforePublishing(t *testing.T) { analyzerFactory := conflictmock.NewMockFactory(ctrl) analyzerFactory.EXPECT().For(conflict.Config{QueueName: request.Queue}).Return(all.New(), nil) controller := NewController( - zaptest.NewLogger(t).Sugar(), tally.NoopScope, registry, cnt, storageFactoryFor(ctrl, store), analyzerFactory, + zaptest.NewLogger(t).Sugar(), tally.NoopScope, registry, staticCounterFactory{counter: cnt}, storageFactoryFor(ctrl, store), analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch", ) @@ -1008,8 +1015,8 @@ func TestController_Process_RedeliveryMintsFreshBatchID(t *testing.T) { secondRequest.Version = 2 cnt := countermock.NewMockCounter(ctrl) - cnt.EXPECT().Next(gomock.Any(), "batch/"+firstRequest.Queue).Return(int64(1), nil) - cnt.EXPECT().Next(gomock.Any(), "batch/"+firstRequest.Queue).Return(int64(2), nil) + cnt.EXPECT().Next(gomock.Any(), counterDomainBatch).Return(int64(1), nil) + cnt.EXPECT().Next(gomock.Any(), counterDomainBatch).Return(int64(2), nil) requestStore := storagemock.NewMockRequestStore(ctrl) requestStore.EXPECT().Get(gomock.Any(), firstRequest.ID).Return(firstRequest, nil) diff --git a/submitqueue/orchestrator/controller/mergeconflictsignal/mergeconflictsignal.go b/submitqueue/orchestrator/controller/mergeconflictsignal/mergeconflictsignal.go index 8819773a9..4bb4315bd 100644 --- a/submitqueue/orchestrator/controller/mergeconflictsignal/mergeconflictsignal.go +++ b/submitqueue/orchestrator/controller/mergeconflictsignal/mergeconflictsignal.go @@ -141,7 +141,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er } request.Version = newVersion - logEntry := entity.NewRequestLog(request.ID, entity.RequestStatusValidated, request.Version, "", nil) + logEntry := entity.NewRequestLog(request.Queue, request.ID, entity.RequestStatusValidated, request.Version, "", nil) if err := corerequest.PublishLog(ctx, c.registry, logEntry, request.ID); err != nil { metrics.NamedCounter(c.metricsScope, opName, "log_errors", 1) return fmt.Errorf("failed to publish request log for %s: %w", request.ID, err) @@ -188,7 +188,7 @@ func (c *Controller) failRequest(ctx context.Context, store storage.Storage, req request.Version = newVersion } - logEntry := entity.NewRequestLog(request.ID, entity.RequestStatusError, request.Version, reason, nil) + logEntry := entity.NewRequestLog(request.Queue, request.ID, entity.RequestStatusError, request.Version, reason, nil) if err := corerequest.PublishLog(ctx, c.registry, logEntry, request.ID); err != nil { return fmt.Errorf("failed to publish request log for %s: %w", request.ID, err) } diff --git a/submitqueue/orchestrator/controller/start/start.go b/submitqueue/orchestrator/controller/start/start.go index 2b4c2dedf..0d0349dd0 100644 --- a/submitqueue/orchestrator/controller/start/start.go +++ b/submitqueue/orchestrator/controller/start/start.go @@ -118,7 +118,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er } // Record the "new" status in the request log. - logEntry := entity.NewRequestLog(request.ID, entity.RequestStatusStarted, request.Version, "", nil) + logEntry := entity.NewRequestLog(request.Queue, request.ID, entity.RequestStatusStarted, request.Version, "", nil) if err := corerequest.PublishLog(ctx, c.registry, logEntry, request.ID); err != nil { metrics.NamedCounter(c.metricsScope, opName, "request_log_errors", 1) return fmt.Errorf("failed to publish request log: %w", err) diff --git a/submitqueue/orchestrator/pipeline.go b/submitqueue/orchestrator/pipeline.go index 1ba3c14cd..a9c777da5 100644 --- a/submitqueue/orchestrator/pipeline.go +++ b/submitqueue/orchestrator/pipeline.go @@ -59,8 +59,8 @@ type Deps struct { // Storage resolves the queue-scoped store aggregate per queue. Storage storage.Factory - // Counter provides distributed batch counters. - Counter counter.Counter + // Counter resolves the queue-scoped batch counter per queue. + Counter counter.Factory // BuildRunner resolves the build runner for each queue. BuildRunner buildrunner.Factory diff --git a/test/e2e/submitqueue/harness_test.go b/test/e2e/submitqueue/harness_test.go index fc1a7c1ed..41830fa78 100644 --- a/test/e2e/submitqueue/harness_test.go +++ b/test/e2e/submitqueue/harness_test.go @@ -49,10 +49,18 @@ func pollUntil(interval time.Duration, condition func() bool) { } } -// land submits a request with the default REBASE strategy and returns its sqid. +// request identifies one landed request. A sqid is only resolvable within its own +// queue, so the read APIs take both and the harness carries the pair together +// rather than threading a bare sqid. +type request struct { + queue string + sqid string +} + +// land submits a request with the default REBASE strategy and returns its identity. // URIs may carry "sq-fake=" markers to steer negative paths (see // platform/fakemarker); the happy path uses a plain change URI. -func (s *E2EIntegrationSuite) land(queue string, uris ...string) string { +func (s *E2EIntegrationSuite) land(queue string, uris ...string) request { t := s.T() resp, err := s.gatewayClient.Land(s.ctx, &gatewaypb.LandRequest{ Queue: queue, @@ -61,47 +69,47 @@ func (s *E2EIntegrationSuite) land(queue string, uris ...string) string { }) require.NoError(t, err, "Land failed for queue %s", queue) require.NotEmpty(t, resp.Sqid, "Land returned an empty sqid for queue %s", queue) - return resp.Sqid + return request{queue: queue, sqid: resp.Sqid} } // currentStatus reads the request's current customer-facing status via // GetRequestSummaryByID. A transport error is returned so callers can keep polling. -func (s *E2EIntegrationSuite) currentStatus(sqid string) (entity.RequestStatus, error) { - resp, err := s.gatewayClient.GetRequestSummaryByID(s.ctx, &gatewaypb.GetRequestSummaryByIDRequest{Sqid: sqid}) +func (s *E2EIntegrationSuite) currentStatus(req request) (entity.RequestStatus, error) { + resp, err := s.gatewayClient.GetRequestSummaryByID(s.ctx, &gatewaypb.GetRequestSummaryByIDRequest{Sqid: req.sqid, Queue: req.queue}) if err != nil { return entity.RequestStatusUnknown, err } if resp.Request == nil { - return entity.RequestStatusUnknown, fmt.Errorf("GetRequestSummaryByID(%s) returned no request", sqid) + return entity.RequestStatusUnknown, fmt.Errorf("GetRequestSummaryByID(%s) returned no request", req.sqid) } return entity.RequestStatus(resp.Request.Status), nil } // awaitStatus polls GetRequestSummaryByID until the request reaches exactly want. -func (s *E2EIntegrationSuite) awaitStatus(sqid string, want entity.RequestStatus) { +func (s *E2EIntegrationSuite) awaitStatus(req request, want entity.RequestStatus) { pollUntil(persistPollInterval, func() bool { - got, err := s.currentStatus(sqid) + got, err := s.currentStatus(req) if err != nil { - s.log.Logf("GetRequestSummaryByID(%s) not ready yet: %v", sqid, err) + s.log.Logf("GetRequestSummaryByID(%s) not ready yet: %v", req.sqid, err) return false } - s.log.Logf("GetRequestSummaryByID(%s) = %q (want %q)", sqid, got, want) + s.log.Logf("GetRequestSummaryByID(%s) = %q (want %q)", req.sqid, got, want) return got == want }) } // awaitTerminal polls GetRequestSummaryByID until the request reaches a terminal status // (landed, error, or cancelled) and returns it. -func (s *E2EIntegrationSuite) awaitTerminal(sqid string) entity.RequestStatus { +func (s *E2EIntegrationSuite) awaitTerminal(req request) entity.RequestStatus { var last entity.RequestStatus pollUntil(persistPollInterval, func() bool { - got, err := s.currentStatus(sqid) + got, err := s.currentStatus(req) if err != nil { - s.log.Logf("GetRequestSummaryByID(%s) not ready yet: %v", sqid, err) + s.log.Logf("GetRequestSummaryByID(%s) not ready yet: %v", req.sqid, err) return false } last = got - s.log.Logf("GetRequestSummaryByID(%s) = %q (awaiting terminal)", sqid, got) + s.log.Logf("GetRequestSummaryByID(%s) = %q (awaiting terminal)", req.sqid, got) return isTerminalStatus(got) }) return last @@ -109,10 +117,10 @@ func (s *E2EIntegrationSuite) awaitTerminal(sqid string) entity.RequestStatus { // timeline returns the ordered customer-facing status history through // GetRequestHistoryByID. -func (s *E2EIntegrationSuite) timeline(sqid string) []entity.RequestStatus { +func (s *E2EIntegrationSuite) timeline(req request) []entity.RequestStatus { t := s.T() - resp, err := s.gatewayClient.GetRequestHistoryByID(s.ctx, &gatewaypb.GetRequestHistoryByIDRequest{Sqid: sqid}) - require.NoError(t, err, "GetRequestHistoryByID failed for %s", sqid) + resp, err := s.gatewayClient.GetRequestHistoryByID(s.ctx, &gatewaypb.GetRequestHistoryByIDRequest{Sqid: req.sqid, Queue: req.queue}) + require.NoError(t, err, "GetRequestHistoryByID failed for %s", req.sqid) statuses := make([]entity.RequestStatus, len(resp.Events)) for i, event := range resp.Events { statuses[i] = entity.RequestStatus(event.Status) @@ -124,9 +132,9 @@ func (s *E2EIntegrationSuite) timeline(sqid string) []entity.RequestStatus { // the GetRequestHistoryByID status timeline. It tolerates intermediate statuses (so it is // not a change-detector), asserting only the relative order of the statuses that // matter. -func (s *E2EIntegrationSuite) assertStatusesInOrder(sqid string, want ...entity.RequestStatus) { +func (s *E2EIntegrationSuite) assertStatusesInOrder(req request, want ...entity.RequestStatus) { t := s.T() - got := s.timeline(sqid) + got := s.timeline(req) matched := 0 for _, st := range got { if matched < len(want) && st == want[matched] { @@ -135,17 +143,17 @@ func (s *E2EIntegrationSuite) assertStatusesInOrder(sqid string, want ...entity. } assert.Equalf(t, len(want), matched, "GetRequestHistoryByID for %s should contain %v as an ordered subsequence; got %v", - sqid, want, got) + req.sqid, want, got) } // assertStatusesNever asserts that none of the banned statuses ever appeared // in the GetRequestHistoryByID status timeline. -func (s *E2EIntegrationSuite) assertStatusesNever(sqid string, banned ...entity.RequestStatus) { +func (s *E2EIntegrationSuite) assertStatusesNever(req request, banned ...entity.RequestStatus) { t := s.T() - got := s.timeline(sqid) + got := s.timeline(req) for _, b := range banned { assert.NotContainsf(t, got, b, - "GetRequestHistoryByID for %s must never contain %q; got %v", sqid, b, got) + "GetRequestHistoryByID for %s must never contain %q; got %v", req.sqid, b, got) } } @@ -218,20 +226,20 @@ func (s *E2EIntegrationSuite) awaitUnparked(consumerGroup, topic, messageID stri // operating store (mysql-app). Unlike the status timeline, RequestState is // point-in-time — the Request entity is updated in place under optimistic // locking, so only the current (terminal, once settled) value is observable. -func (s *E2EIntegrationSuite) terminalState(queue, sqid string) entity.RequestState { +func (s *E2EIntegrationSuite) terminalState(req request) entity.RequestState { t := s.T() - store, err := s.appStorage.For(queue) - require.NoError(t, err, "failed to resolve operating store for queue %s", queue) - req, err := store.GetRequestStore().Get(s.ctx, sqid) - require.NoError(t, err, "failed to get request %s from operating store", sqid) - return req.State + store, err := s.appStorage.For(req.queue) + require.NoError(t, err, "failed to resolve operating store for queue %s", req.queue) + got, err := store.GetRequestStore().Get(s.ctx, req.sqid) + require.NoError(t, err, "failed to get request %s from operating store", req.sqid) + return got.State } // lastError returns the LastError reported by GetRequestSummaryByID. -func (s *E2EIntegrationSuite) lastError(sqid string) string { +func (s *E2EIntegrationSuite) lastError(req request) string { t := s.T() - resp, err := s.gatewayClient.GetRequestSummaryByID(s.ctx, &gatewaypb.GetRequestSummaryByIDRequest{Sqid: sqid}) - require.NoError(t, err, "GetRequestSummaryByID failed for %s", sqid) + resp, err := s.gatewayClient.GetRequestSummaryByID(s.ctx, &gatewaypb.GetRequestSummaryByIDRequest{Sqid: req.sqid, Queue: req.queue}) + require.NoError(t, err, "GetRequestSummaryByID failed for %s", req.sqid) require.NotNil(t, resp.Request) return resp.Request.LastError } diff --git a/test/e2e/submitqueue/suite_test.go b/test/e2e/submitqueue/suite_test.go index f77ad84d7..b2d57a17a 100644 --- a/test/e2e/submitqueue/suite_test.go +++ b/test/e2e/submitqueue/suite_test.go @@ -227,18 +227,18 @@ func (s *E2EIntegrationSuite) TestPingOrchestrator() { // storage only via that cross-service publish→consume→persist path, so its // presence in GetRequestHistoryByID proves the path works. func (s *E2EIntegrationSuite) TestLand_HappyPath_ReachesLanded() { - sqid := s.land("e2e-test-queue", "github://github.example.com/uber/e2e-service/pull/123/abcdef0123456789abcdef0123456789abcdef01") - s.log.Logf("Land (happy path) succeeded: sqid=%s; waiting for landed", sqid) + req := s.land("e2e-test-queue", "github://github.example.com/uber/e2e-service/pull/123/abcdef0123456789abcdef0123456789abcdef01") + s.log.Logf("Land (happy path) succeeded: sqid=%s; waiting for landed", req.sqid) // Black-box: the customer-facing status reaches landed. - s.awaitStatus(sqid, entity.RequestStatusLanded) + s.awaitStatus(req, entity.RequestStatusLanded) // Black-box history: all status entries for a request share its request_id // partition on the log topic, and the terminal "landed" is published last. // Once "landed" is observed, GetRequestHistoryByID must expose the earlier statuses. // This is a tolerant ordered-subsequence match because the pipeline does not // emit every possible display status. - s.assertStatusesInOrder(sqid, + s.assertStatusesInOrder(req, entity.RequestStatusAccepted, entity.RequestStatusStarted, entity.RequestStatusBatched, @@ -248,8 +248,8 @@ func (s *E2EIntegrationSuite) TestLand_HappyPath_ReachesLanded() { // White-box (internal state): the operating store's authoritative // RequestState settled on landed. RequestState is point-in-time, so this is a // terminal check, not a sequence. - assert.Equal(s.T(), entity.RequestStateLanded, s.terminalState("e2e-test-queue", sqid), - "operating store should show request %s in terminal state landed", sqid) + assert.Equal(s.T(), entity.RequestStateLanded, s.terminalState(req), + "operating store should show request %s in terminal state landed", req.sqid) } // TestReadAPIs validates all five request read endpoints against receipts @@ -260,24 +260,26 @@ func (s *E2EIntegrationSuite) TestReadAPIs() { queue = "e2e-test-queue" changeURI = "github://uber/e2e-read-apis/pull/456/abcdef0123456789abcdef0123456789abcdef01" ) - firstSqid := s.land(queue, changeURI) - secondSqid := s.land(queue, changeURI) - s.awaitStatus(firstSqid, entity.RequestStatusLanded) - s.awaitStatus(secondSqid, entity.RequestStatusError) + first := s.land(queue, changeURI) + second := s.land(queue, changeURI) + firstSqid, secondSqid := first.sqid, second.sqid - firstSummary, err := s.gatewayClient.GetRequestSummaryByID(s.ctx, &gatewaypb.GetRequestSummaryByIDRequest{Sqid: firstSqid}) + s.awaitStatus(first, entity.RequestStatusLanded) + s.awaitStatus(second, entity.RequestStatusError) + + firstSummary, err := s.gatewayClient.GetRequestSummaryByID(s.ctx, &gatewaypb.GetRequestSummaryByIDRequest{Sqid: firstSqid, Queue: queue}) require.NoError(t, err) require.NotNil(t, firstSummary.Request) assert.Equal(t, firstSqid, firstSummary.Request.Sqid) assert.Equal(t, queue, firstSummary.Request.Queue) assert.Equal(t, []string{changeURI}, firstSummary.Request.ChangeUris) - secondSummary, err := s.gatewayClient.GetRequestSummaryByID(s.ctx, &gatewaypb.GetRequestSummaryByIDRequest{Sqid: secondSqid}) + secondSummary, err := s.gatewayClient.GetRequestSummaryByID(s.ctx, &gatewaypb.GetRequestSummaryByIDRequest{Sqid: secondSqid, Queue: queue}) require.NoError(t, err) require.NotNil(t, secondSummary.Request) assert.Contains(t, secondSummary.Request.LastError, firstSqid) - summariesByChange, err := s.gatewayClient.GetRequestSummaryByChangeURI(s.ctx, &gatewaypb.GetRequestSummaryByChangeURIRequest{ChangeUri: changeURI}) + summariesByChange, err := s.gatewayClient.GetRequestSummaryByChangeURI(s.ctx, &gatewaypb.GetRequestSummaryByChangeURIRequest{ChangeUri: changeURI, Queue: queue}) require.NoError(t, err) require.Len(t, summariesByChange.Requests, 2) expectedNewestFirst := []string{firstSqid, secondSqid} @@ -312,12 +314,12 @@ func (s *E2EIntegrationSuite) TestReadAPIs() { } assert.Equal(t, expectedNewestFirst, listedSqids) - historyByID, err := s.gatewayClient.GetRequestHistoryByID(s.ctx, &gatewaypb.GetRequestHistoryByIDRequest{Sqid: firstSqid}) + historyByID, err := s.gatewayClient.GetRequestHistoryByID(s.ctx, &gatewaypb.GetRequestHistoryByIDRequest{Sqid: firstSqid, Queue: queue}) require.NoError(t, err) require.NotEmpty(t, historyByID.Events) assert.Equal(t, string(entity.RequestStatusAccepted), historyByID.Events[0].Status) - historyByChange, err := s.gatewayClient.GetRequestHistoryByChangeURI(s.ctx, &gatewaypb.GetRequestHistoryByChangeURIRequest{ChangeUri: changeURI}) + historyByChange, err := s.gatewayClient.GetRequestHistoryByChangeURI(s.ctx, &gatewaypb.GetRequestHistoryByChangeURIRequest{ChangeUri: changeURI, Queue: queue}) require.NoError(t, err) require.Len(t, historyByChange.Histories, 2) assert.Equal(t, []string{firstSqid, secondSqid}, []string{historyByChange.Histories[0].Sqid, historyByChange.Histories[1].Sqid}) @@ -376,10 +378,10 @@ func (s *E2EIntegrationSuite) TestCancel_CaughtPreBatch_NeverLands() { // stack with a delivery still parked. Opening twice is a no-op. defer s.openGate(gateGroup, queue) - sqid := s.land(queue, "github://github.example.com/uber/e2e-cancel/pull/9999/abcdef0123456789abcdef0123456789abcdef01") - s.log.Logf("Land (cancel path) succeeded: sqid=%s; awaiting parked check", sqid) + req := s.land(queue, "github://github.example.com/uber/e2e-cancel/pull/9999/abcdef0123456789abcdef0123456789abcdef01") + s.log.Logf("Land (cancel path) succeeded: sqid=%s; awaiting parked check", req.sqid) - parked := s.awaitParked(gateGroup, gateTopic, sqid) + parked := s.awaitParked(gateGroup, gateTopic, req.sqid) assert.Equal(t, queue, parked.PartitionKey, "check message should be partitioned by queue") assert.NotEmpty(t, parked.Payload, "parked record should carry the check payload") @@ -387,21 +389,21 @@ func (s *E2EIntegrationSuite) TestCancel_CaughtPreBatch_NeverLands() { // cancel now. The request cannot be batched until the check is answered, // so the cancel controller takes the not-batched path to terminal // Cancelled. - _, err := s.gatewayClient.Cancel(s.ctx, &gatewaypb.CancelRequest{Sqid: sqid, Reason: "e2e cancel test"}) + _, err := s.gatewayClient.Cancel(s.ctx, &gatewaypb.CancelRequest{Sqid: req.sqid, Queue: queue, Reason: "e2e cancel test"}) require.NoError(t, err, "Cancel failed") - s.awaitStatus(sqid, entity.RequestStatusCancelled) - s.assertStatusesInOrder(sqid, + s.awaitStatus(req, entity.RequestStatusCancelled) + s.assertStatusesInOrder(req, entity.RequestStatusAccepted, entity.RequestStatusCancelling, entity.RequestStatusCancelled, ) - assert.Equal(t, entity.RequestStateCancelled, s.terminalState(queue, sqid), - "operating store should show request %s terminal cancelled while its check is parked", sqid) + assert.Equal(t, entity.RequestStateCancelled, s.terminalState(req), + "operating store should show request %s terminal cancelled while its check is parked", req.sqid) // Start the controller again and prove the parked delivery cleared the gate. s.openGate(gateGroup, queue) - s.awaitUnparked(gateGroup, gateTopic, sqid) + s.awaitUnparked(gateGroup, gateTopic, req.sqid) // Sentinel on the same queue: its landing proves the stale signal ahead of // it on the same partitions was consumed. @@ -409,7 +411,7 @@ func (s *E2EIntegrationSuite) TestCancel_CaughtPreBatch_NeverLands() { s.awaitStatus(sentinel, entity.RequestStatusLanded) // The stale check answer was dropped: the cancelled request never advanced. - assert.Equal(t, entity.RequestStateCancelled, s.terminalState(queue, sqid), - "request %s must stay terminal cancelled after its stale check signal is processed", sqid) - s.assertStatusesNever(sqid, entity.RequestStatusBatched, entity.RequestStatusLanded) + assert.Equal(t, entity.RequestStateCancelled, s.terminalState(req), + "request %s must stay terminal cancelled after its stale check signal is processed", req.sqid) + s.assertStatusesNever(req, entity.RequestStatusBatched, entity.RequestStatusLanded) } diff --git a/test/integration/extension/counter/mysql/BUILD.bazel b/test/integration/extension/counter/mysql/BUILD.bazel index def70a25f..7728804b0 100644 --- a/test/integration/extension/counter/mysql/BUILD.bazel +++ b/test/integration/extension/counter/mysql/BUILD.bazel @@ -12,6 +12,7 @@ go_test( "requires-network", ], deps = [ + "//platform/extension/counter:go_default_library", "//platform/extension/counter/mysql:go_default_library", "//test/integration/extension/counter:go_default_library", "//test/testutil:go_default_library", diff --git a/test/integration/extension/counter/mysql/counter_test.go b/test/integration/extension/counter/mysql/counter_test.go index 6787b4c54..532a6331e 100644 --- a/test/integration/extension/counter/mysql/counter_test.go +++ b/test/integration/extension/counter/mysql/counter_test.go @@ -23,11 +23,21 @@ import ( "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "github.com/uber-go/tally" + "github.com/uber/submitqueue/platform/extension/counter" mysqlcounter "github.com/uber/submitqueue/platform/extension/counter/mysql" countersuite "github.com/uber/submitqueue/test/integration/extension/counter" "github.com/uber/submitqueue/test/testutil" ) +// counterFactory binds the shared MySQL pool to a queue, mirroring how the service +// wiring adapts the backend to the counter.Factory seam. +type counterFactory struct{ db *sql.DB } + +// For returns the Counter bound to the queue named in config. +func (f counterFactory) For(config counter.Config) (counter.Counter, error) { + return mysqlcounter.NewCounter(f.db, tally.NoopScope, config.QueueName), nil +} + // MySQLCounterIntegrationSuite tests the MySQL counter implementation // by embedding the shared contract suite. type MySQLCounterIntegrationSuite struct { @@ -73,12 +83,12 @@ func (s *MySQLCounterIntegrationSuite) SetupSuite() { s.log.Logf("Schemas applied successfully") - // Create counter instance - cnt := mysqlcounter.NewCounter(s.db, tally.NoopScope) + // Create counter factory + fty := counterFactory{db: s.db} - // Provide the counter instance to the contract suite + // Provide the counter factory to the contract suite s.SetContext(ctx) - s.SetCounter(cnt) + s.SetFactory(fty) s.SetLogger(s.log) t.Cleanup(func() { diff --git a/test/integration/extension/counter/suite.go b/test/integration/extension/counter/suite.go index f2679fc56..be42e12f9 100644 --- a/test/integration/extension/counter/suite.go +++ b/test/integration/extension/counter/suite.go @@ -25,12 +25,14 @@ import ( "github.com/uber/submitqueue/test/testutil" ) -// CounterContractSuite defines the contract tests for the counter.Counter interface. +// CounterContractSuite defines the contract tests for the counter extension: the +// queue-scoped Counter resolved through counter.Factory. // All counter implementations must pass these tests. -// Implementation-specific tests should embed this suite and call SetCounter(). +// Implementation-specific tests should embed this suite and call SetFactory(). type CounterContractSuite struct { suite.Suite ctx context.Context + factory counter.Factory counter counter.Counter log *testutil.TestLogger } @@ -40,9 +42,18 @@ func (s *CounterContractSuite) SetContext(ctx context.Context) { s.ctx = ctx } -// SetCounter is called by implementation tests to provide the concrete counter instance -func (s *CounterContractSuite) SetCounter(c counter.Counter) { - s.counter = c +// SetFactory is called by implementation tests to provide the queue-scoped counter +// factory under test. The suite's single-queue tests run against a default binding. +func (s *CounterContractSuite) SetFactory(f counter.Factory) { + s.factory = f + s.counter = s.forQueue("contract-queue") +} + +// forQueue resolves the counter bound to a queue, failing the test on resolution errors. +func (s *CounterContractSuite) forQueue(queue string) counter.Counter { + c, err := s.factory.For(counter.Config{QueueName: queue}) + s.Require().NoError(err) + return c } // SetLogger sets the logger for tests @@ -137,3 +148,33 @@ func (s *CounterContractSuite) TestCounter_Concurrency() { "sequences should be contiguous at index %d: got %d and %d", i, sequences[i-1], sequences[i]) } } + +// TestCounter_QueueIsolation tests that the same domain in two queues is two +// independent sequences: the queue leads the key, so one queue's counter must +// never advance or observe another's. +func (s *CounterContractSuite) TestCounter_QueueIsolation() { + t := s.T() + ctx := s.ctx + + const domain = "request" + queueA := s.forQueue("isolation-queue-a") + queueB := s.forQueue("isolation-queue-b") + + a1, err := queueA.Next(ctx, domain) + require.NoError(t, err) + a2, err := queueA.Next(ctx, domain) + require.NoError(t, err) + assert.Equal(t, a1+1, a2, "queue A advances its own sequence") + + // Queue B starts its own sequence rather than continuing A's. + b1, err := queueB.Next(ctx, domain) + require.NoError(t, err) + assert.Equal(t, a1, b1, "the same domain in another queue is an independent sequence") + + // Advancing B leaves A untouched. + _, err = queueB.Next(ctx, domain) + require.NoError(t, err) + a3, err := queueA.Next(ctx, domain) + require.NoError(t, err) + assert.Equal(t, a2+1, a3, "queue B's writes must not advance queue A") +} diff --git a/test/integration/submitqueue/extension/storage/mysql/storage_test.go b/test/integration/submitqueue/extension/storage/mysql/storage_test.go index 9bf58f3d6..dda5a6871 100644 --- a/test/integration/submitqueue/extension/storage/mysql/storage_test.go +++ b/test/integration/submitqueue/extension/storage/mysql/storage_test.go @@ -78,11 +78,10 @@ func (s *MySQLStorageIntegrationSuite) SetupSuite() { store, err := mysqlstorage.NewStorage(s.db, tally.NoopScope) require.NoError(t, err, "failed to create storage") - // Provide the storage backend to the contract suite: the queue-scoped - // factory adapter plus the global read-model stores. + // Provide the storage backend to the contract suite through the + // queue-scoped factory adapter. s.SetContext(ctx) s.SetFactory(mysqlFactory{backend: store}) - s.SetGlobalStores(store.GetRequestSummaryStore(), store.GetRequestURIStore()) s.SetLogger(s.log) t.Cleanup(func() { diff --git a/test/integration/submitqueue/extension/storage/suite.go b/test/integration/submitqueue/extension/storage/suite.go index 928d81213..b406308ab 100644 --- a/test/integration/submitqueue/extension/storage/suite.go +++ b/test/integration/submitqueue/extension/storage/suite.go @@ -31,17 +31,14 @@ import ( ) // StorageContractSuite defines the contract tests for the storage extension: -// the queue-scoped aggregate resolved through storage.Factory plus the global -// read-model stores. All storage implementations must pass these tests. -// Implementation-specific tests should embed this suite and call SetFactory() -// and SetGlobalStores(). +// the queue-scoped aggregate resolved through storage.Factory. All storage +// implementations must pass these tests. Implementation-specific tests should +// embed this suite and call SetFactory(). type StorageContractSuite struct { suite.Suite - ctx context.Context - factory storage.Factory - summaries storage.RequestSummaryStore - uris storage.RequestURIStore - log *testutil.TestLogger + ctx context.Context + factory storage.Factory + log *testutil.TestLogger } // SetContext sets the context for tests @@ -55,13 +52,6 @@ func (s *StorageContractSuite) SetFactory(factory storage.Factory) { s.factory = factory } -// SetGlobalStores is called by implementation tests to provide the global -// read-model stores under test. -func (s *StorageContractSuite) SetGlobalStores(summaries storage.RequestSummaryStore, uris storage.RequestURIStore) { - s.summaries = summaries - s.uris = uris -} - // forQueue resolves the queue-scoped store aggregate for a queue, failing the // test on resolution errors. func (s *StorageContractSuite) forQueue(queue string) storage.Storage { @@ -668,12 +658,13 @@ func (s *StorageContractSuite) TestStorage_BuildCreateAndGet() { func (s *StorageContractSuite) TestStorage_RequestSummaryCreateGetAndCAS() { t := s.T() ctx := s.ctx + const queue = "summary-q" summary := entity.RequestSummary{ - RequestID: "summary/1", Queue: "summary-q", ChangeURIs: nil, ReceivedAtMs: 100, + RequestID: "summary/1", Queue: queue, ChangeURIs: nil, ReceivedAtMs: 100, Status: entity.RequestStatusAccepted, RequestVersion: 1, StatusTimestampMs: 100, Version: 1, LastError: "", Metadata: nil, } - store := s.summaries + store := s.forQueue(queue).GetRequestSummaryStore() require.NoError(t, store.Create(ctx, summary)) require.ErrorIs(t, store.Create(ctx, summary), storage.ErrAlreadyExists) @@ -685,7 +676,6 @@ func (s *StorageContractSuite) TestStorage_RequestSummaryCreateGetAndCAS() { _, err = store.Get(ctx, "summary/missing") require.ErrorIs(t, err, storage.ErrNotFound) - got.Queue = "summary-q-updated" got.ChangeURIs = []string{"change/updated"} got.ReceivedAtMs = 200 got.Status = entity.RequestStatusLanded @@ -699,7 +689,7 @@ func (s *StorageContractSuite) TestStorage_RequestSummaryCreateGetAndCAS() { require.NoError(t, err) assert.Equal(t, entity.RequestSummary{ RequestID: summary.RequestID, - Queue: "summary-q-updated", + Queue: queue, ChangeURIs: []string{"change/updated"}, ReceivedAtMs: 200, Status: entity.RequestStatusLanded, @@ -710,8 +700,9 @@ func (s *StorageContractSuite) TestStorage_RequestSummaryCreateGetAndCAS() { Metadata: map[string]string{"source": "test"}, }, updated) + // A stale version is rejected. The queue stays the bound one: it is part of the + // key now, so a summary cannot be moved between queues by an update. stale := updated - stale.Queue = "stale-q" stale.ChangeURIs = []string{"change/stale"} stale.ReceivedAtMs = 400 stale.Status = entity.RequestStatusError @@ -721,6 +712,11 @@ func (s *StorageContractSuite) TestStorage_RequestSummaryCreateGetAndCAS() { stale.Metadata = map[string]string{"source": "stale"} require.ErrorIs(t, store.Update(ctx, stale, 1, 3), storage.ErrVersionMismatch) + // A summary naming another queue is rejected by the binding outright. + otherQueue := updated + otherQueue.Queue = "summary-q-other" + assert.Error(t, store.Update(ctx, otherQueue, 2, 3)) + afterStale, err := store.Get(ctx, summary.RequestID) require.NoError(t, err) assert.Equal(t, updated, afterStale) @@ -846,11 +842,12 @@ func (s *StorageContractSuite) TestStorage_RequestQueueSummaryListAndCursor() { func (s *StorageContractSuite) TestStorage_RequestURIListIsBoundedAndOrdered() { t := s.T() ctx := s.ctx - store := s.uris + const queue = "uri-q" + store := s.forQueue(queue).GetRequestURIStore() rows := []entity.RequestURI{ - {ChangeURI: "uri/shared", ReceivedAtMs: 100, RequestID: "uri/1"}, - {ChangeURI: "uri/shared", ReceivedAtMs: 200, RequestID: "uri/2"}, - {ChangeURI: "uri/shared", ReceivedAtMs: 200, RequestID: "uri/3"}, + {ChangeURI: "uri/shared", Queue: queue, ReceivedAtMs: 100, RequestID: "uri/1"}, + {ChangeURI: "uri/shared", Queue: queue, ReceivedAtMs: 200, RequestID: "uri/2"}, + {ChangeURI: "uri/shared", Queue: queue, ReceivedAtMs: 200, RequestID: "uri/3"}, } for _, row := range rows { require.NoError(t, store.Create(ctx, row)) @@ -865,11 +862,63 @@ func (s *StorageContractSuite) TestStorage_RequestURIListIsBoundedAndOrdered() { empty, err := store.ListByURI(ctx, "uri/missing", 2) require.NoError(t, err) assert.Empty(t, empty) + + // The same change URI in another queue is a distinct mapping set. + otherStore := s.forQueue("uri-q-other").GetRequestURIStore() + require.NoError(t, otherStore.Create(ctx, entity.RequestURI{ + ChangeURI: "uri/shared", Queue: "uri-q-other", ReceivedAtMs: 100, RequestID: "other/1", + }), "the same change URI in another queue is a distinct row") + otherGot, err := otherStore.ListByURI(ctx, "uri/shared", 10) + require.NoError(t, err) + require.Len(t, otherGot, 1, "one queue's mappings must not surface through another's binding") + assert.Equal(t, "other/1", otherGot[0].RequestID) +} + +// TestStorage_RequestLogAppendAndList tests the append-only audit trail and its +// queue scoping: a request ID is unique only within its queue, so the same ID in +// two queues is two independent histories. +func (s *StorageContractSuite) TestStorage_RequestLogAppendAndList() { + t := s.T() + ctx := s.ctx + const queue = "log-q" + store := s.forQueue(queue).GetRequestLogStore() + + _, err := store.List(ctx, "log/missing") + require.ErrorIs(t, err, storage.ErrNotFound) + + entries := []entity.RequestLog{ + {RequestID: "log/1", Queue: queue, TimestampMs: 100, Status: entity.RequestStatusAccepted, Metadata: map[string]string{}}, + {RequestID: "log/1", Queue: queue, TimestampMs: 200, Status: entity.RequestStatusStarted, LastError: "detail", Metadata: map[string]string{"k": "v"}}, + } + for _, entry := range entries { + require.NoError(t, store.Insert(ctx, entry)) + } + + got, err := store.List(ctx, "log/1") + require.NoError(t, err) + require.Len(t, got, 2) + assert.Equal(t, entity.RequestStatusAccepted, got[0].Status, "entries come back in timestamp order") + assert.Equal(t, entity.RequestStatusStarted, got[1].Status) + + // A log carrying another queue's name is rejected by the binding. + assert.Error(t, store.Insert(ctx, entity.RequestLog{ + RequestID: "log/1", Queue: "log-q-other", TimestampMs: 300, Status: entity.RequestStatusLanded, Metadata: map[string]string{}, + })) + + // The same request ID in another queue is an independent history. + otherStore := s.forQueue("log-q-other").GetRequestLogStore() + require.NoError(t, otherStore.Insert(ctx, entity.RequestLog{ + RequestID: "log/1", Queue: "log-q-other", TimestampMs: 150, Status: entity.RequestStatusLanded, Metadata: map[string]string{}, + })) + otherGot, err := otherStore.List(ctx, "log/1") + require.NoError(t, err) + require.Len(t, otherGot, 1, "one queue's log must not surface through another's binding") + assert.Equal(t, entity.RequestStatusLanded, otherGot[0].Status) } // speculationPathSet builds a two-path set for head over one dependency: one // path assuming the dependency succeeds, one assuming it fails. -func speculationPathSet(head, dep string) entity.SpeculationPathSet { +func speculationPathSet(queue, head, dep string) entity.SpeculationPathSet { succeeds := entity.SpeculationPath{ Head: head, Dependencies: []entity.PathDependency{{Batch: dep, Assumption: entity.DependencyAssumptionSucceeds}}, @@ -879,7 +928,8 @@ func speculationPathSet(head, dep string) entity.SpeculationPathSet { Dependencies: []entity.PathDependency{{Batch: dep, Assumption: entity.DependencyAssumptionFails}}, } return entity.SpeculationPathSet{ - Head: head, + Queue: queue, + Head: head, Paths: []entity.SpeculationPathEntry{ { ID: succeeds.ID(), @@ -912,7 +962,7 @@ func (s *StorageContractSuite) TestStorage_SpeculationPathSetCreateAndGet() { ctx := s.ctx store := s.forQueue("test-queue").GetSpeculationPathSetStore() - want := speculationPathSet("sps/head/1", "sps/dep/1") + want := speculationPathSet("test-queue", "sps/head/1", "sps/dep/1") require.NoError(t, store.Create(ctx, want)) got, err := store.Get(ctx, want.Head) @@ -939,7 +989,7 @@ func (s *StorageContractSuite) TestStorage_SpeculationPathSetCreateDuplicate() { ctx := s.ctx store := s.forQueue("test-queue").GetSpeculationPathSetStore() - set := speculationPathSet("sps/head/duplicate", "sps/dep/1") + set := speculationPathSet("test-queue", "sps/head/duplicate", "sps/dep/1") require.NoError(t, store.Create(ctx, set)) assert.ErrorIs(t, store.Create(ctx, set), storage.ErrAlreadyExists) } @@ -952,7 +1002,7 @@ func (s *StorageContractSuite) TestStorage_SpeculationPathSetOptimisticLocking() ctx := s.ctx store := s.forQueue("test-queue").GetSpeculationPathSetStore() - set := speculationPathSet("sps/head/cas", "sps/dep/1") + set := speculationPathSet("test-queue", "sps/head/cas", "sps/dep/1") require.NoError(t, store.Create(ctx, set)) // Winner: replaces the set under the version it read. @@ -972,3 +1022,32 @@ func (s *StorageContractSuite) TestStorage_SpeculationPathSetOptimisticLocking() require.Len(t, got.Paths, 1, "the losing write must not have restored the dropped path") assert.Equal(t, entity.SpeculationPathStatusPassed, got.Paths[0].Status) } + +// TestStorage_SpeculationPathSetQueueIsolation tests that the same head in two +// queues is two independent sets: a batch ID is only unique within its queue, so +// one queue's binding must never surface or overwrite another's row. +func (s *StorageContractSuite) TestStorage_SpeculationPathSetQueueIsolation() { + t := s.T() + ctx := s.ctx + + const head = "sps/head/shared" + storeA := s.forQueue("queue-a").GetSpeculationPathSetStore() + storeB := s.forQueue("queue-b").GetSpeculationPathSetStore() + + setA := speculationPathSet("queue-a", head, "sps/dep/a") + setB := speculationPathSet("queue-b", head, "sps/dep/b") + require.NoError(t, storeA.Create(ctx, setA)) + require.NoError(t, storeB.Create(ctx, setB), "the same head in another queue is a distinct row") + + gotA, err := storeA.Get(ctx, head) + require.NoError(t, err) + assert.Equal(t, setA, gotA) + + gotB, err := storeB.Get(ctx, head) + require.NoError(t, err) + assert.Equal(t, setB, gotB) + + // A set carrying another queue's name is rejected by the binding. + assert.Error(t, storeA.Create(ctx, setB)) + assert.Error(t, storeA.Update(ctx, setB, 1, 2)) +} diff --git a/test/integration/submitqueue/gateway/suite_test.go b/test/integration/submitqueue/gateway/suite_test.go index c7b3528a4..7b0599df6 100644 --- a/test/integration/submitqueue/gateway/suite_test.go +++ b/test/integration/submitqueue/gateway/suite_test.go @@ -172,16 +172,19 @@ func (s *GatewayIntegrationSuite) TestListAPI() { t := s.T() store, err := mysqlstorage.NewStorage(s.db, tally.NoopScope) require.NoError(t, err) - materializer := corerequest.NewMaterializer(store.GetRequestLogStore(), store.GetRequestSummaryStore(), store.GetRequestURIStore(), mysqlFactory{backend: store}) + materializer := corerequest.NewMaterializer(mysqlFactory{backend: store}) + queueStore, err := store.For("test-queue") + require.NoError(t, err) for _, summary := range []entity.RequestSummary{ {RequestID: "test-queue/list-1", Queue: "test-queue", ChangeURIs: []string{"uri/1"}, ReceivedAtMs: 100, Status: entity.RequestStatusAccepted, StatusTimestampMs: 100, Version: 1, Metadata: map[string]string{}}, {RequestID: "test-queue/list-2", Queue: "test-queue", ChangeURIs: []string{"uri/2"}, ReceivedAtMs: 200, Status: entity.RequestStatusLanded, StatusTimestampMs: 200, Version: 1, Metadata: map[string]string{}}, } { publicStatus := summary.Status summary.Status = entity.RequestStatusAccepting - require.NoError(t, store.GetRequestSummaryStore().Create(s.ctx, summary)) + require.NoError(t, queueStore.GetRequestSummaryStore().Create(s.ctx, summary)) require.NoError(t, materializer.PersistLog(s.ctx, entity.RequestLog{ RequestID: summary.RequestID, + Queue: summary.Queue, TimestampMs: summary.StatusTimestampMs, Status: publicStatus, Metadata: map[string]string{}, @@ -205,7 +208,7 @@ func (s *GatewayIntegrationSuite) TestListAPI() { func (s *GatewayIntegrationSuite) TestReadAPIErrorCodes() { t := s.T() - _, err := s.client.GetRequestSummaryByID(s.ctx, &pb.GetRequestSummaryByIDRequest{Sqid: "missing/1"}) + _, err := s.client.GetRequestSummaryByID(s.ctx, &pb.GetRequestSummaryByIDRequest{Sqid: "missing/1", Queue: "missing"}) require.Error(t, err) assert.Equal(t, codes.NotFound, status.Code(err)) @@ -220,29 +223,35 @@ func (s *GatewayIntegrationSuite) TestReadAPIErrorCodes() { store, err := mysqlstorage.NewStorage(s.db, tally.NoopScope) require.NoError(t, err) const overflowChangeURI = "uri/read-api-overflow" + overflowStore, err := store.For("overflow") + require.NoError(t, err) for i := 1; i <= 101; i++ { - require.NoError(t, store.GetRequestURIStore().Create(s.ctx, entity.RequestURI{ + require.NoError(t, overflowStore.GetRequestURIStore().Create(s.ctx, entity.RequestURI{ ChangeURI: overflowChangeURI, + Queue: "overflow", ReceivedAtMs: int64(i), RequestID: fmt.Sprintf("overflow/%d", i), })) } - _, err = s.client.GetRequestSummaryByChangeURI(s.ctx, &pb.GetRequestSummaryByChangeURIRequest{ChangeUri: overflowChangeURI}) + _, err = s.client.GetRequestSummaryByChangeURI(s.ctx, &pb.GetRequestSummaryByChangeURIRequest{ChangeUri: overflowChangeURI, Queue: "overflow"}) require.Error(t, err) assert.Equal(t, codes.ResourceExhausted, status.Code(err)) - _, err = s.client.GetRequestHistoryByChangeURI(s.ctx, &pb.GetRequestHistoryByChangeURIRequest{ChangeUri: overflowChangeURI}) + _, err = s.client.GetRequestHistoryByChangeURI(s.ctx, &pb.GetRequestHistoryByChangeURIRequest{ChangeUri: overflowChangeURI, Queue: "overflow"}) require.Error(t, err) assert.Equal(t, codes.ResourceExhausted, status.Code(err)) const inconsistentChangeURI = "uri/read-api-inconsistent" - require.NoError(t, store.GetRequestURIStore().Create(s.ctx, entity.RequestURI{ + inconsistentStore, err := store.For("missing-summary") + require.NoError(t, err) + require.NoError(t, inconsistentStore.GetRequestURIStore().Create(s.ctx, entity.RequestURI{ ChangeURI: inconsistentChangeURI, + Queue: "missing-summary", ReceivedAtMs: 1, RequestID: "missing-summary/1", })) - _, err = s.client.GetRequestSummaryByChangeURI(s.ctx, &pb.GetRequestSummaryByChangeURIRequest{ChangeUri: inconsistentChangeURI}) + _, err = s.client.GetRequestSummaryByChangeURI(s.ctx, &pb.GetRequestSummaryByChangeURIRequest{ChangeUri: inconsistentChangeURI, Queue: "missing-summary"}) require.Error(t, err) assert.Equal(t, codes.Internal, status.Code(err)) } @@ -273,21 +282,24 @@ func (s *GatewayIntegrationSuite) TestRequestLogConsumer() { require.NoError(t, err, "failed to create topic registry") const sqid = "log-consumer-test/1" + const logQueue = "log-consumer-test" store, err := mysqlstorage.NewStorage(s.db, tally.NoopScope) require.NoError(t, err) + logQueueStore, err := store.For(logQueue) + require.NoError(t, err) summary := entity.RequestSummary{ - RequestID: sqid, Queue: "log-consumer-test", ChangeURIs: []string{}, ReceivedAtMs: 1, + RequestID: sqid, Queue: logQueue, ChangeURIs: []string{}, ReceivedAtMs: 1, Status: entity.RequestStatusAccepting, StatusTimestampMs: 1, Version: 1, Metadata: map[string]string{}, } - require.NoError(t, store.GetRequestSummaryStore().Create(s.ctx, summary)) - logEntry := entity.NewRequestLog(sqid, entity.RequestStatusStarted, 1, "", nil) + require.NoError(t, logQueueStore.GetRequestSummaryStore().Create(s.ctx, summary)) + logEntry := entity.NewRequestLog(logQueue, sqid, entity.RequestStatusStarted, 1, "", nil) require.NoError(t, corerequest.PublishLog(s.ctx, registry, logEntry, sqid), "failed to publish request log to log topic") s.log.Logf("Published 'started' log for sqid=%s; waiting for gateway consumer to persist it", sqid) require.Eventually(t, func() bool { - resp, statusErr := s.client.GetRequestSummaryByID(s.ctx, &pb.GetRequestSummaryByIDRequest{Sqid: sqid}) + resp, statusErr := s.client.GetRequestSummaryByID(s.ctx, &pb.GetRequestSummaryByIDRequest{Sqid: sqid, Queue: "log-consumer-test"}) if statusErr != nil { return false } diff --git a/tool/linter/queueshard/BUILD.bazel b/tool/linter/queueshard/BUILD.bazel new file mode 100644 index 000000000..3bbf2670e --- /dev/null +++ b/tool/linter/queueshard/BUILD.bazel @@ -0,0 +1,24 @@ +load("@rules_go//go:def.bzl", "go_binary", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = ["main.go"], + importpath = "github.com/uber/submitqueue/tool/linter/queueshard", + visibility = ["//visibility:private"], +) + +go_binary( + name = "queueshard", + embed = [":go_default_library"], + visibility = ["//visibility:public"], +) + +go_test( + name = "go_default_test", + srcs = ["main_test.go"], + embed = [":go_default_library"], + deps = [ + "@com_github_stretchr_testify//assert:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + ], +) diff --git a/tool/linter/queueshard/main.go b/tool/linter/queueshard/main.go new file mode 100644 index 000000000..3b32f95f2 --- /dev/null +++ b/tool/linter/queueshard/main.go @@ -0,0 +1,198 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Command queueshard checks that every domain table is shardable by queue: its +// primary key must lead with the queue column, and no secondary index may span +// queues. +// +// A table is shardable by queue when one queue's rows are unreachable through +// another queue's binding. That holds exactly when the queue is the leading +// primary-key column, because every read is then a primary-key-prefix scan +// within one queue. A secondary index that does not itself lead with the queue +// reintroduces a cross-queue access path, so those are rejected too. +package main + +import ( + "flag" + "fmt" + "os" + "path/filepath" + "regexp" + "strings" +) + +// queueColumns are the column names that identify the owning queue. Most tables +// call it "queue"; a table whose rows *are* queues (stovepipe's queue table) +// names it "name" because the row's identity is the queue itself. +var queueColumns = map[string]bool{ + "queue": true, + "name": true, +} + +// schemaRoots are the directories scanned for table definitions. +// +// platform/extension/messagequeue is deliberately absent: it is a message-queue +// backend keyed by (consumer_group, topic, partition_key), not a domain table +// set, and sharding it is tracked separately. +var schemaRoots = []string{ + "submitqueue/extension/storage/mysql/schema", + "stovepipe/extension/storage/mysql/schema", + "platform/extension/counter/mysql/schema", +} + +var ( + createTableRe = regexp.MustCompile(`(?is)CREATE\s+TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?` + "`?" + `(\w+)` + "`?" + `\s*\((.*)\)\s*ENGINE`) + primaryKeyRe = regexp.MustCompile(`(?i)PRIMARY\s+KEY\s*\(([^)]*)\)`) + indexRe = regexp.MustCompile(`(?im)^\s*(?:UNIQUE\s+)?(?:KEY|INDEX)\s+` + "`?" + `(\w+)` + "`?" + `\s*\(([^)]*)\)`) +) + +// violation is one table that is not shardable by queue. +type violation struct { + file string + table string + problem string +} + +func main() { + flag.Parse() + + root, err := findRepoRoot() + if err != nil { + fmt.Fprintf(os.Stderr, "error: %v\n", err) + os.Exit(1) + } + + var violations []violation + var checked int + for _, schemaRoot := range schemaRoots { + files, err := filepath.Glob(filepath.Join(root, schemaRoot, "*.sql")) + if err != nil { + fmt.Fprintf(os.Stderr, "error globbing %s: %v\n", schemaRoot, err) + os.Exit(1) + } + if len(files) == 0 { + fmt.Fprintf(os.Stderr, "error: no .sql files under %s\n", schemaRoot) + os.Exit(1) + } + for _, file := range files { + content, err := os.ReadFile(file) + if err != nil { + fmt.Fprintf(os.Stderr, "error reading %s: %v\n", file, err) + os.Exit(1) + } + rel, relErr := filepath.Rel(root, file) + if relErr != nil { + rel = file + } + found, tableViolations := check(rel, string(content)) + checked += found + violations = append(violations, tableViolations...) + } + } + + if len(violations) > 0 { + fmt.Fprintf(os.Stderr, "%d table(s) are not shardable by queue:\n\n", len(violations)) + for _, v := range violations { + fmt.Fprintf(os.Stderr, " %s: table %q %s\n", v.file, v.table, v.problem) + } + fmt.Fprintf(os.Stderr, "\nEvery table's primary key must lead with the queue column, and no\n") + fmt.Fprintf(os.Stderr, "secondary index may span queues, so that one queue's rows are\n") + fmt.Fprintf(os.Stderr, "unreachable through another queue's binding.\n") + os.Exit(1) + } + + fmt.Printf("All %d tables are shardable by queue.\n", checked) +} + +// check returns the number of tables found in content and any violations. +func check(file, content string) (int, []violation) { + var violations []violation + matches := createTableRe.FindAllStringSubmatch(content, -1) + for _, match := range matches { + table, body := match[1], match[2] + + pk := primaryKeyRe.FindStringSubmatch(body) + if pk == nil { + violations = append(violations, violation{file, table, "has no PRIMARY KEY"}) + continue + } + columns := splitColumns(pk[1]) + if len(columns) == 0 { + violations = append(violations, violation{file, table, "has an empty PRIMARY KEY"}) + continue + } + if !queueColumns[columns[0]] { + violations = append(violations, violation{ + file, table, + fmt.Sprintf("leads its PRIMARY KEY with %q, not the queue column", columns[0]), + }) + } + + for _, idx := range indexRe.FindAllStringSubmatch(body, -1) { + idxColumns := splitColumns(idx[2]) + if len(idxColumns) == 0 || !queueColumns[idxColumns[0]] { + lead := "(empty)" + if len(idxColumns) > 0 { + lead = idxColumns[0] + } + violations = append(violations, violation{ + file, table, + fmt.Sprintf("has index %q leading with %q, which spans queues", idx[1], lead), + }) + } + } + } + return len(matches), violations +} + +// splitColumns parses a comma-separated column list, stripping backticks, +// whitespace, and any length prefix such as `col(20)`. +func splitColumns(list string) []string { + var columns []string + for _, raw := range strings.Split(list, ",") { + col := strings.TrimSpace(raw) + col = strings.Trim(col, "`") + if idx := strings.IndexByte(col, '('); idx >= 0 { + col = col[:idx] + } + col = strings.TrimSpace(col) + if col != "" { + columns = append(columns, col) + } + } + return columns +} + +// findRepoRoot walks up from the working directory to the module root. +func findRepoRoot() (string, error) { + // Bazel `run` executes from the runfiles tree; BUILD_WORKSPACE_DIRECTORY + // points back at the source tree. + if dir := os.Getenv("BUILD_WORKSPACE_DIRECTORY"); dir != "" { + return dir, nil + } + dir, err := os.Getwd() + if err != nil { + return "", err + } + for { + if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil { + return dir, nil + } + parent := filepath.Dir(dir) + if parent == dir { + return "", fmt.Errorf("could not find repository root (no go.mod found)") + } + dir = parent + } +} diff --git a/tool/linter/queueshard/main_test.go b/tool/linter/queueshard/main_test.go new file mode 100644 index 000000000..2b5887b4f --- /dev/null +++ b/tool/linter/queueshard/main_test.go @@ -0,0 +1,148 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCheck(t *testing.T) { + tests := []struct { + name string + schema string + wantTables int + wantViolations int + wantProblem string + }{ + { + name: "queue-leading composite key passes", + schema: "CREATE TABLE IF NOT EXISTS request (\n" + + " queue VARCHAR(255) NOT NULL,\n" + + " id VARCHAR(255) NOT NULL,\n" + + " PRIMARY KEY (queue, id)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;", + wantTables: 1, + }, + { + name: "queue-free key is rejected", + schema: "CREATE TABLE IF NOT EXISTS request_summary (\n" + + " request_id VARCHAR(255) NOT NULL,\n" + + " queue VARCHAR(255) NOT NULL,\n" + + " PRIMARY KEY (request_id)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;", + wantTables: 1, + wantViolations: 1, + wantProblem: `leads its PRIMARY KEY with "request_id", not the queue column`, + }, + { + name: "queue present but not leading is rejected", + schema: "CREATE TABLE IF NOT EXISTS batch (\n" + + " id VARCHAR(255) NOT NULL,\n" + + " queue VARCHAR(255) NOT NULL,\n" + + " PRIMARY KEY (id, queue)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;", + wantTables: 1, + wantViolations: 1, + }, + { + name: "a table of queues may key on name", + schema: "CREATE TABLE IF NOT EXISTS queue (\n" + + " name VARCHAR(255) NOT NULL,\n" + + " PRIMARY KEY (name)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;", + wantTables: 1, + }, + { + name: "backticked table and columns are parsed", + schema: "CREATE TABLE IF NOT EXISTS `change` (\n" + + " `queue` VARCHAR(255) NOT NULL,\n" + + " `uri` VARCHAR(255) NOT NULL,\n" + + " PRIMARY KEY (`queue`, `uri`)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;", + wantTables: 1, + }, + { + name: "a secondary index spanning queues is rejected", + schema: "CREATE TABLE IF NOT EXISTS build (\n" + + " queue VARCHAR(255) NOT NULL,\n" + + " id VARCHAR(255) NOT NULL,\n" + + " status VARCHAR(64) NOT NULL,\n" + + " PRIMARY KEY (queue, id),\n" + + " INDEX idx_status (status)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;", + wantTables: 1, + wantViolations: 1, + wantProblem: `has index "idx_status" leading with "status", which spans queues`, + }, + { + name: "a queue-leading secondary index passes", + schema: "CREATE TABLE IF NOT EXISTS build (\n" + + " queue VARCHAR(255) NOT NULL,\n" + + " id VARCHAR(255) NOT NULL,\n" + + " status VARCHAR(64) NOT NULL,\n" + + " PRIMARY KEY (queue, id),\n" + + " INDEX idx_queue_status (queue, status)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;", + wantTables: 1, + }, + { + name: "a table with no primary key is rejected", + schema: "CREATE TABLE IF NOT EXISTS loose (\n" + + " queue VARCHAR(255) NOT NULL\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;", + wantTables: 1, + wantViolations: 1, + wantProblem: "has no PRIMARY KEY", + }, + { + name: "leading comments are ignored", + schema: "-- a comment about the table\nCREATE TABLE IF NOT EXISTS t (\n queue VARCHAR(255) NOT NULL,\n PRIMARY KEY (queue)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;", + wantTables: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tables, violations := check("test.sql", tt.schema) + assert.Equal(t, tt.wantTables, tables) + require.Len(t, violations, tt.wantViolations) + if tt.wantProblem != "" { + assert.Equal(t, tt.wantProblem, violations[0].problem) + } + }) + } +} + +func TestSplitColumns(t *testing.T) { + tests := []struct { + name string + list string + want []string + }{ + {name: "plain", list: "queue, id", want: []string{"queue", "id"}}, + {name: "backticked", list: "`queue`, `id`", want: []string{"queue", "id"}}, + {name: "length prefix", list: "queue(20), id", want: []string{"queue", "id"}}, + {name: "empty", list: "", want: nil}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, splitColumns(tt.list)) + }) + } +}