diff --git a/api/runway/messagequeue/proto/merge.proto b/api/runway/messagequeue/proto/merge.proto index babd70ab..5ab44e55 100644 --- a/api/runway/messagequeue/proto/merge.proto +++ b/api/runway/messagequeue/proto/merge.proto @@ -126,4 +126,7 @@ message MergeResult { // merge populates each step's outputs with the revisions it produced; a // dry-run check leaves them empty. repeated StepResult steps = 4; + // queue_name echoes the caller-provided queue name from the request, so the + // consumer can route the result by queue without loading state first. + string queue_name = 5; } diff --git a/api/runway/messagequeue/protopb/merge.pb.go b/api/runway/messagequeue/protopb/merge.pb.go index 668e26e3..e1bbd6ae 100644 --- a/api/runway/messagequeue/protopb/merge.pb.go +++ b/api/runway/messagequeue/protopb/merge.pb.go @@ -378,7 +378,10 @@ type MergeResult struct { // steps optionally reports per-step outcomes, in request order. A committing // merge populates each step's outputs with the revisions it produced; a // dry-run check leaves them empty. - Steps []*StepResult `protobuf:"bytes,4,rep,name=steps,proto3" json:"steps,omitempty"` + Steps []*StepResult `protobuf:"bytes,4,rep,name=steps,proto3" json:"steps,omitempty"` + // queue_name echoes the caller-provided queue name from the request, so the + // consumer can route the result by queue without loading state first. + QueueName string `protobuf:"bytes,5,opt,name=queue_name,json=queueName,proto3" json:"queue_name,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -441,6 +444,13 @@ func (x *MergeResult) GetSteps() []*StepResult { return nil } +func (x *MergeResult) GetQueueName() string { + if x != nil { + return x.QueueName + } + return "" +} + var File_merge_proto protoreflect.FileDescriptor const file_merge_proto_rawDesc = "" + @@ -462,12 +472,14 @@ const file_merge_proto_rawDesc = "" + "StepResult\x12\x17\n" + "\astep_id\x18\x01 \x01(\tR\x06stepId\x12>\n" + "\aoutputs\x18\x02 \x03(\v2$.uber.runway.messagequeue.StepOutputR\aoutputs\x12\x16\n" + - "\x06reason\x18\x03 \x01(\tR\x06reason\"\xdf\x01\n" + + "\x06reason\x18\x03 \x01(\tR\x06reason\"\xfe\x01\n" + "\vMergeResult\x12\x0e\n" + "\x02id\x18\x01 \x01(\tR\x02id\x12;\n" + "\aoutcome\x18\x02 \x01(\x0e2!.uber.runway.messagequeue.OutcomeR\aoutcome\x12\x16\n" + "\x06reason\x18\x03 \x01(\tR\x06reason\x12:\n" + - "\x05steps\x18\x04 \x03(\v2$.uber.runway.messagequeue.StepResultR\x05steps:/\x8a\xb5\x18\x1bmerge-conflict-check-signal\x8a\xb5\x18\fmerge-signal*=\n" + + "\x05steps\x18\x04 \x03(\v2$.uber.runway.messagequeue.StepResultR\x05steps\x12\x1d\n" + + "\n" + + "queue_name\x18\x05 \x01(\tR\tqueueName:/\x8a\xb5\x18\x1bmerge-conflict-check-signal\x8a\xb5\x18\fmerge-signal*=\n" + "\aOutcome\x12\x17\n" + "\x13OUTCOME_UNSPECIFIED\x10\x00\x12\r\n" + "\tSUCCEEDED\x10\x01\x12\n" + diff --git a/doc/rfc/submitqueue/extension-contract.md b/doc/rfc/submitqueue/extension-contract.md index 15d006cb..e2ebfb0e 100644 --- a/doc/rfc/submitqueue/extension-contract.md +++ b/doc/rfc/submitqueue/extension-contract.md @@ -14,7 +14,7 @@ Both unblock with the shape `conflict` already uses: accept identity, resolve in ## Principle - **Decision/action extensions** take orchestrator identity at their stage granularity and resolve granular content through narrowly-injected dependencies. Request stage → `entity.Request`; batch stage → `entity.Batch` / `[]entity.Batch`. Both are thin reference entities (a `Request` carries URIs, not diffs; a `Batch` carries IDs, not changes). -- **Resolution targets** — `storage`, `changestore`, `queueconfig` — stay key/value-shaped. They are what the others resolve *through* (see [storage/README.md](../../../submitqueue/extension/storage/README.md) and CLAUDE.md). +- **Resolution targets** — `storage`, `changestore`, `queueconfig` — stay key/value-shaped. They are what the others resolve *through* (see [storage/README.md](../../../submitqueue/extension/storage/README.md) and CLAUDE.md). Refinement: the storage *aggregate* has since gained the same per-queue factory resolution every other seam has — the stores it hands back remain strictly key/value, bound to their queue, while the cross-queue read-model stores stay individually-injected singletons. - **Output mirrors the input unit.** Each output element self-identifies with the input it corresponds to — `changeprovider`'s `ChangeInfo` carries its `URI`, `conflict`'s `Conflict` carries its `BatchID` — so a flat list suffices and the caller correlates results back to inputs without re-deriving boundaries. A *wrapper* entity (`entity.BatchChanges`) is introduced only to aggregate *up* to a coarser unit than the elements — the scorer needs batch-wide line/file totals, so the rollup earns its keep; no `RequestChanges` exists because nothing needs request-wide rollups. And when the input is a *collection* of independently-actioned units, the output groups by them: `pusher`, fed `[]entity.Batch`, returns outcomes grouped per batch, the same way `conflict` already tags each `Conflict` with its in-flight `BatchID`. ### What each stage resolves today diff --git a/runway/controller/dlq/dlq.go b/runway/controller/dlq/dlq.go index 64f5832c..c36ed38b 100644 --- a/runway/controller/dlq/dlq.go +++ b/runway/controller/dlq/dlq.go @@ -137,9 +137,10 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er ) result := &runwaymq.MergeResult{ - Id: request.GetId(), - Outcome: runwaypb.Outcome_FAILED, - Reason: fmt.Sprintf("dead-lettered: %s", reason), + Id: request.GetId(), + Outcome: runwaypb.Outcome_FAILED, + Reason: fmt.Sprintf("dead-lettered: %s", reason), + QueueName: request.GetQueueName(), } if err := c.publish(ctx, result, msg.PartitionKey); err != nil { diff --git a/runway/controller/merge/merge.go b/runway/controller/merge/merge.go index 1d78c9d1..9dc6e074 100644 --- a/runway/controller/merge/merge.go +++ b/runway/controller/merge/merge.go @@ -128,6 +128,10 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er } } + // Echo the request's queue name so the consumer can route the result by + // queue without loading state first. + result.QueueName = request.GetQueueName() + if err := c.publish(ctx, runwaymq.TopicKeyMergeSignal, result, msg.PartitionKey); err != nil { metrics.NamedCounter(c.metricsScope, opName, "publish_errors", 1) return fmt.Errorf("failed to publish merge result for %s: %w", request.GetId(), err) diff --git a/runway/controller/mergeconflictcheck/mergeconflictcheck.go b/runway/controller/mergeconflictcheck/mergeconflictcheck.go index 8ccfa017..87e3d5ba 100644 --- a/runway/controller/mergeconflictcheck/mergeconflictcheck.go +++ b/runway/controller/mergeconflictcheck/mergeconflictcheck.go @@ -128,6 +128,10 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er } } + // Echo the request's queue name so the consumer can route the result by + // queue without loading state first. + result.QueueName = request.GetQueueName() + if err := c.publish(ctx, runwaymq.TopicKeyMergeConflictCheckSignal, result, msg.PartitionKey); err != nil { metrics.NamedCounter(c.metricsScope, opName, "publish_errors", 1) return fmt.Errorf("failed to publish merge-conflict-check result for %s: %w", request.GetId(), err) diff --git a/service/submitqueue/gateway/server/BUILD.bazel b/service/submitqueue/gateway/server/BUILD.bazel index 8452288f..71e23a70 100644 --- a/service/submitqueue/gateway/server/BUILD.bazel +++ b/service/submitqueue/gateway/server/BUILD.bazel @@ -23,8 +23,10 @@ go_library( "//platform/extension/messagequeue:go_default_library", "//platform/extension/messagequeue/mysql:go_default_library", "//service/submitqueue/gateway/server/mapper:go_default_library", + "//submitqueue/core/request:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/extension/queueconfig/yaml:go_default_library", + "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mysql:go_default_library", "//submitqueue/gateway/controller:go_default_library", "//submitqueue/gateway/controller/log:go_default_library", diff --git a/service/submitqueue/gateway/server/main.go b/service/submitqueue/gateway/server/main.go index 8186b707..f60bdaca 100644 --- a/service/submitqueue/gateway/server/main.go +++ b/service/submitqueue/gateway/server/main.go @@ -40,8 +40,10 @@ import ( extqueue "github.com/uber/submitqueue/platform/extension/messagequeue" queueMySQL "github.com/uber/submitqueue/platform/extension/messagequeue/mysql" "github.com/uber/submitqueue/service/submitqueue/gateway/server/mapper" + requestcore "github.com/uber/submitqueue/submitqueue/core/request" "github.com/uber/submitqueue/submitqueue/core/topickey" yamlqueueconfig "github.com/uber/submitqueue/submitqueue/extension/queueconfig/yaml" + "github.com/uber/submitqueue/submitqueue/extension/storage" mysqlstorage "github.com/uber/submitqueue/submitqueue/extension/storage/mysql" "github.com/uber/submitqueue/submitqueue/gateway/controller" logctrl "github.com/uber/submitqueue/submitqueue/gateway/controller/log" @@ -321,17 +323,21 @@ func run() error { return fmt.Errorf("failed to load queue configs: %w", err) } - // Create controllers and wrap them for gRPC + // 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. + storageFty := storageFactory{backend: store} + materializer := requestcore.NewMaterializer(store.GetRequestLogStore(), store.GetRequestSummaryStore(), store.GetRequestURIStore(), storageFty) pingController := controller.NewPingController(logger, scope) - landController := controller.NewLandController(logger.Sugar(), scope, cnt, store, queueConfigs, registry) - cancelController := controller.NewCancelController(logger.Sugar(), scope, store, registry) + 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(), ) - listController := controller.NewListController(logger.Sugar(), scope, store.GetRequestQueueSummaryStore(), queueConfigs) + listController := controller.NewListController(logger.Sugar(), scope, storageFty, queueConfigs) requestHistoryController := controller.NewRequestHistoryController( logger.Sugar(), scope, @@ -367,7 +373,7 @@ func run() error { newConsumerGate(logger), ) - logController := logctrl.NewController(logger.Sugar(), scope, store, topickey.TopicKeyLog, "gateway-log") + logController := logctrl.NewController(logger.Sugar(), scope, materializer, topickey.TopicKeyLog, "gateway-log") if err := logConsumer.Register(logController); err != nil { return fmt.Errorf("failed to register log controller: %w", err) } @@ -457,3 +463,16 @@ func newConsumerGate(logger *zap.Logger) consumergate.Gate { logger.Info("consumer gate configured", zap.String("dir", dir)) return consumergatefile.New(dir) } + +// storageFactory adapts the MySQL storage backend's queue binding to the +// storage.Factory seam. Routing every queue to the single shared backend is +// this host's policy; a deployment that splits queues across backends swaps +// this adapter for a routing one. +type storageFactory struct { + backend *mysqlstorage.Storage +} + +// For returns the queue-scoped store aggregate bound to the queue named in config. +func (f storageFactory) For(config storage.Config) (storage.Storage, error) { + return f.backend.For(config.QueueName) +} diff --git a/service/submitqueue/orchestrator/server/BUILD.bazel b/service/submitqueue/orchestrator/server/BUILD.bazel index e197655a..3e7ad937 100644 --- a/service/submitqueue/orchestrator/server/BUILD.bazel +++ b/service/submitqueue/orchestrator/server/BUILD.bazel @@ -37,6 +37,7 @@ go_library( "//submitqueue/extension/conflict/fake:go_default_library", "//submitqueue/extension/conflict/fileoverlap:go_default_library", "//submitqueue/extension/conflict/none:go_default_library", + "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mysql:go_default_library", "//submitqueue/extension/validator/fake:go_default_library", "//submitqueue/orchestrator:go_default_library", diff --git a/service/submitqueue/orchestrator/server/main.go b/service/submitqueue/orchestrator/server/main.go index 1fb01444..d76ce7a9 100644 --- a/service/submitqueue/orchestrator/server/main.go +++ b/service/submitqueue/orchestrator/server/main.go @@ -47,6 +47,7 @@ import ( githubprovider "github.com/uber/submitqueue/submitqueue/extension/changeprovider/github" phabprovider "github.com/uber/submitqueue/submitqueue/extension/changeprovider/phabricator" routingprovider "github.com/uber/submitqueue/submitqueue/extension/changeprovider/routing" + "github.com/uber/submitqueue/submitqueue/extension/storage" mysqlstorage "github.com/uber/submitqueue/submitqueue/extension/storage/mysql" validatorfake "github.com/uber/submitqueue/submitqueue/extension/validator/fake" "github.com/uber/submitqueue/submitqueue/orchestrator" @@ -180,7 +181,8 @@ func run() error { // Build per-queue extension profiles (host-private). Each queue resolves // to its own set of extension implementations (conflict analyzer, …), // falling back to a baseline profile for queues without an explicit entry. - profiles, err := newProfiles(logger, scope, changeset.New(store.GetRequestStore(), store.GetChangeStore())) + storageFty := storageFactory{backend: store} + profiles, err := newProfiles(logger, scope, changeset.New(storageFty)) if err != nil { return fmt.Errorf("failed to build profiles: %w", err) } @@ -191,7 +193,7 @@ func run() error { deps := orchestrator.Deps{ Logger: logger.Sugar(), Scope: scope, - Storage: store, + Storage: storageFty, Counter: cnt, BuildRunner: profiles.BuildRunnerFactory(), ChangeProvider: profiles.ChangeProviderFactory(), @@ -431,3 +433,16 @@ func parseTimeout(envVal string, defaultVal time.Duration) time.Duration { } return defaultVal } + +// storageFactory adapts the MySQL storage backend's queue binding to the +// storage.Factory seam. Routing every queue to the single shared backend is +// this host's policy; a deployment that splits queues across backends swaps +// this adapter for a routing one. +type storageFactory struct { + backend *mysqlstorage.Storage +} + +// For returns the queue-scoped store aggregate bound to the queue named in config. +func (f storageFactory) For(config storage.Config) (storage.Storage, error) { + return f.backend.For(config.QueueName) +} diff --git a/submitqueue/core/batch/list.go b/submitqueue/core/batch/list.go index edbc0f64..edd2cb7c 100644 --- a/submitqueue/core/batch/list.go +++ b/submitqueue/core/batch/list.go @@ -28,9 +28,9 @@ import ( // ListByStates call issues while hydrating candidate IDs. const hydrateConcurrency = 16 -// ListByStates returns the queue's batches whose current state is one of the given -// states, read through the queue's membership records: each requested state bucket -// is listed, candidate IDs are deduplicated across buckets, every candidate is +// ListByStates returns the bound queue's batches whose current state is one of the +// given states, read through the queue's membership records: each requested state +// bucket is listed, candidate IDs are deduplicated across buckets, every candidate is // hydrated by key with bounded concurrency, and the result keeps only batches whose // hydrated State is in states. Classification always uses the hydrated state — a // record found in a stale bucket can therefore never misreport a batch, only route @@ -39,7 +39,7 @@ const hydrateConcurrency = 16 // A candidate ID whose batch does not exist is returned as an error rather than // skipped: batch rows are never deleted, so a dangling record means the store is // inconsistent, not that the batch concluded. -func ListByStates(ctx context.Context, store storage.Storage, queue string, states []entity.BatchState) ([]entity.Batch, error) { +func ListByStates(ctx context.Context, store storage.Storage, states []entity.BatchState) ([]entity.Batch, error) { wanted := make(map[entity.BatchState]bool, len(states)) seen := make(map[string]bool) var ids []string @@ -49,9 +49,9 @@ func ListByStates(ctx context.Context, store storage.Storage, queue string, stat } wanted[state] = true - records, err := store.GetQueueBatchStateStore().List(ctx, queue, state) + records, err := store.GetQueueBatchStateStore().List(ctx, state) if err != nil { - return nil, fmt.Errorf("failed to list queue batch state records for queue %s state %s: %w", queue, state, err) + return nil, fmt.Errorf("failed to list queue batch state records for state %s: %w", state, err) } for _, record := range records { if seen[record.BatchID] { @@ -69,7 +69,7 @@ func ListByStates(ctx context.Context, store storage.Storage, queue string, stat g.Go(func() error { batch, err := store.GetBatchStore().Get(gctx, id) if err != nil { - return fmt.Errorf("failed to get batch %s of queue %s: %w", id, queue, err) + return fmt.Errorf("failed to get batch %s: %w", id, err) } hydrated[i] = batch return nil diff --git a/submitqueue/core/batch/list_test.go b/submitqueue/core/batch/list_test.go index 6a632138..8ae14bd4 100644 --- a/submitqueue/core/batch/list_test.go +++ b/submitqueue/core/batch/list_test.go @@ -58,9 +58,9 @@ func TestListByStates(t *testing.T) { setup: func(batchStore *storagemock.MockBatchStore, recordStore *storagemock.MockQueueBatchStateStore) { // b2 appears in both buckets (mid-move duplicate): it must be hydrated // and returned exactly once. - recordStore.EXPECT().List(gomock.Any(), testQueue, entity.BatchStateCreated). + recordStore.EXPECT().List(gomock.Any(), entity.BatchStateCreated). Return([]entity.QueueBatchState{record(entity.BatchStateCreated, "b1"), record(entity.BatchStateCreated, "b2")}, nil) - recordStore.EXPECT().List(gomock.Any(), testQueue, entity.BatchStateSpeculating). + recordStore.EXPECT().List(gomock.Any(), entity.BatchStateSpeculating). Return([]entity.QueueBatchState{record(entity.BatchStateSpeculating, "b2"), record(entity.BatchStateSpeculating, "b3")}, nil) batchStore.EXPECT().Get(gomock.Any(), "b1").Return(batchIn("b1", entity.BatchStateCreated), nil) batchStore.EXPECT().Get(gomock.Any(), "b2").Return(batchIn("b2", entity.BatchStateSpeculating), nil) @@ -77,7 +77,7 @@ func TestListByStates(t *testing.T) { setup: func(batchStore *storagemock.MockBatchStore, recordStore *storagemock.MockQueueBatchStateStore) { // A stale record files b1 under created, but the batch has moved on to // speculating — a state outside the requested set, so it is dropped. - recordStore.EXPECT().List(gomock.Any(), testQueue, entity.BatchStateCreated). + recordStore.EXPECT().List(gomock.Any(), entity.BatchStateCreated). Return([]entity.QueueBatchState{record(entity.BatchStateCreated, "b1")}, nil) batchStore.EXPECT().Get(gomock.Any(), "b1").Return(batchIn("b1", entity.BatchStateSpeculating), nil) }, @@ -87,9 +87,9 @@ func TestListByStates(t *testing.T) { setup: func(batchStore *storagemock.MockBatchStore, recordStore *storagemock.MockQueueBatchStateStore) { // Only a stale created record exists for b1, but its hydrated state is // speculating — requested, so the batch is returned under its true state. - recordStore.EXPECT().List(gomock.Any(), testQueue, entity.BatchStateCreated). + recordStore.EXPECT().List(gomock.Any(), entity.BatchStateCreated). Return([]entity.QueueBatchState{record(entity.BatchStateCreated, "b1")}, nil) - recordStore.EXPECT().List(gomock.Any(), testQueue, entity.BatchStateSpeculating). + recordStore.EXPECT().List(gomock.Any(), entity.BatchStateSpeculating). Return(nil, nil) batchStore.EXPECT().Get(gomock.Any(), "b1").Return(batchIn("b1", entity.BatchStateSpeculating), nil) }, @@ -98,7 +98,7 @@ func TestListByStates(t *testing.T) { "duplicate input states are listed once": { states: []entity.BatchState{entity.BatchStateCreated, entity.BatchStateCreated}, setup: func(batchStore *storagemock.MockBatchStore, recordStore *storagemock.MockQueueBatchStateStore) { - recordStore.EXPECT().List(gomock.Any(), testQueue, entity.BatchStateCreated). + recordStore.EXPECT().List(gomock.Any(), entity.BatchStateCreated). Return([]entity.QueueBatchState{record(entity.BatchStateCreated, "b1")}, nil). Times(1) batchStore.EXPECT().Get(gomock.Any(), "b1").Return(batchIn("b1", entity.BatchStateCreated), nil) @@ -108,14 +108,14 @@ func TestListByStates(t *testing.T) { "list failure surfaces": { states: []entity.BatchState{entity.BatchStateCreated}, setup: func(batchStore *storagemock.MockBatchStore, recordStore *storagemock.MockQueueBatchStateStore) { - recordStore.EXPECT().List(gomock.Any(), testQueue, entity.BatchStateCreated).Return(nil, storeErr) + recordStore.EXPECT().List(gomock.Any(), entity.BatchStateCreated).Return(nil, storeErr) }, wantErr: storeErr, }, "hydrate failure surfaces": { states: []entity.BatchState{entity.BatchStateCreated}, setup: func(batchStore *storagemock.MockBatchStore, recordStore *storagemock.MockQueueBatchStateStore) { - recordStore.EXPECT().List(gomock.Any(), testQueue, entity.BatchStateCreated). + recordStore.EXPECT().List(gomock.Any(), entity.BatchStateCreated). Return([]entity.QueueBatchState{record(entity.BatchStateCreated, "b1")}, nil) batchStore.EXPECT().Get(gomock.Any(), "b1").Return(entity.Batch{}, storeErr) }, @@ -124,7 +124,7 @@ func TestListByStates(t *testing.T) { "dangling record is an error, not a skip": { states: []entity.BatchState{entity.BatchStateCreated}, setup: func(batchStore *storagemock.MockBatchStore, recordStore *storagemock.MockQueueBatchStateStore) { - recordStore.EXPECT().List(gomock.Any(), testQueue, entity.BatchStateCreated). + recordStore.EXPECT().List(gomock.Any(), entity.BatchStateCreated). Return([]entity.QueueBatchState{record(entity.BatchStateCreated, "b1")}, nil) batchStore.EXPECT().Get(gomock.Any(), "b1").Return(entity.Batch{}, storage.WrapNotFound(errors.New("no rows"))) }, @@ -137,7 +137,7 @@ func TestListByStates(t *testing.T) { mockStorage, mockBatchStore, mockRecordStore := testStores(t) tt.setup(mockBatchStore, mockRecordStore) - got, err := ListByStates(context.Background(), mockStorage, testQueue, tt.states) + got, err := ListByStates(context.Background(), mockStorage, tt.states) if tt.wantErr != nil { require.Error(t, err) assert.ErrorIs(t, err, tt.wantErr) diff --git a/submitqueue/core/batch/transition.go b/submitqueue/core/batch/transition.go index 2f442501..32493e7f 100644 --- a/submitqueue/core/batch/transition.go +++ b/submitqueue/core/batch/transition.go @@ -67,7 +67,7 @@ func Transition(ctx context.Context, store storage.Storage, batch entity.Batch, return updated, fmt.Errorf("failed to put queue batch state record for batch %s under state %s: %w", updated.ID, newState, err) } if oldState != newState { - if err := store.GetQueueBatchStateStore().Delete(ctx, updated.Queue, oldState, updated.ID); err != nil { + if err := store.GetQueueBatchStateStore().Delete(ctx, oldState, updated.ID); err != nil { return updated, fmt.Errorf("failed to delete queue batch state record for batch %s under state %s: %w", updated.ID, oldState, err) } } diff --git a/submitqueue/core/batch/transition_test.go b/submitqueue/core/batch/transition_test.go index 3aba102e..b99ff47c 100644 --- a/submitqueue/core/batch/transition_test.go +++ b/submitqueue/core/batch/transition_test.go @@ -67,7 +67,7 @@ func TestTransition(t *testing.T) { recordStore.EXPECT().Put(gomock.Any(), entity.QueueBatchState{ Queue: base.Queue, State: entity.BatchStateSpeculating, BatchID: base.ID, }).Return(nil) - recordStore.EXPECT().Delete(gomock.Any(), base.Queue, entity.BatchStateCreated, base.ID).Return(nil) + recordStore.EXPECT().Delete(gomock.Any(), entity.BatchStateCreated, base.ID).Return(nil) }, want: func() entity.Batch { b := casTarget @@ -115,7 +115,7 @@ func TestTransition(t *testing.T) { setup: func(batchStore *storagemock.MockBatchStore, recordStore *storagemock.MockQueueBatchStateStore) { batchStore.EXPECT().Update(gomock.Any(), casTarget, int32(3), int32(4)).Return(nil) recordStore.EXPECT().Put(gomock.Any(), gomock.Any()).Return(nil) - recordStore.EXPECT().Delete(gomock.Any(), base.Queue, entity.BatchStateCreated, base.ID).Return(storeErr) + recordStore.EXPECT().Delete(gomock.Any(), entity.BatchStateCreated, base.ID).Return(storeErr) }, want: func() entity.Batch { b := casTarget diff --git a/submitqueue/core/changeset/BUILD.bazel b/submitqueue/core/changeset/BUILD.bazel index 11919129..3a73067f 100644 --- a/submitqueue/core/changeset/BUILD.bazel +++ b/submitqueue/core/changeset/BUILD.bazel @@ -22,6 +22,7 @@ go_test( deps = [ "//platform/base/change:go_default_library", "//submitqueue/entity:go_default_library", + "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mock:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", diff --git a/submitqueue/core/changeset/resolver.go b/submitqueue/core/changeset/resolver.go index 049b3a0d..df09e3ea 100644 --- a/submitqueue/core/changeset/resolver.go +++ b/submitqueue/core/changeset/resolver.go @@ -23,25 +23,28 @@ import ( "github.com/uber/submitqueue/submitqueue/extension/storage" ) -// resolver is the store-backed Resolver. It owns the two resolution-target -// stores and nothing else: a request store to walk batch.Contains, and a change -// store to attach provider details for the Detailed view. +// resolver is the store-backed Resolver. It holds the storage factory and +// resolves the batch's queue-scoped request and change stores per call, since +// every resolution is for exactly one batch and the batch names its queue. type resolver struct { - requests storage.RequestStore - changes storage.ChangeStore + stores storage.Factory } -// New returns a Resolver backed by the given request and change stores. -func New(requests storage.RequestStore, changes storage.ChangeStore) Resolver { - return resolver{requests: requests, changes: changes} +// New returns a Resolver backed by the given storage factory. +func New(stores storage.Factory) Resolver { + return resolver{stores: stores} } // ChangesForBatch resolves a batch's requests to their raw changes, in // batch.Contains order. func (r resolver) ChangesForBatch(ctx context.Context, batch entity.Batch) ([]change.Change, error) { + store, err := r.stores.For(storage.Config{QueueName: batch.Queue}) + if err != nil { + return nil, fmt.Errorf("failed to resolve storage for queue %q: %w", batch.Queue, err) + } changes := make([]change.Change, 0, len(batch.Contains)) for _, requestID := range batch.Contains { - request, err := r.requests.Get(ctx, requestID) + request, err := store.GetRequestStore().Get(ctx, requestID) if err != nil { return nil, fmt.Errorf("failed to get request %s for batch %s: %w", requestID, batch.ID, err) } @@ -54,14 +57,18 @@ func (r resolver) ChangesForBatch(ctx context.Context, batch entity.Batch) ([]ch // ChangeInfo per claimed URI, owned by the requesting request, aggregated across // the whole batch. func (r resolver) DetailedForBatch(ctx context.Context, batch entity.Batch) (entity.BatchChanges, error) { + store, err := r.stores.For(storage.Config{QueueName: batch.Queue}) + if err != nil { + return entity.BatchChanges{}, fmt.Errorf("failed to resolve storage for queue %q: %w", batch.Queue, err) + } result := entity.BatchChanges{BatchID: batch.ID, Queue: batch.Queue} for _, requestID := range batch.Contains { - request, err := r.requests.Get(ctx, requestID) + request, err := store.GetRequestStore().Get(ctx, requestID) if err != nil { return entity.BatchChanges{}, fmt.Errorf("failed to get request %s: %w", requestID, err) } for _, uri := range request.Change.URIs { - records, err := r.changes.GetByURI(ctx, batch.Queue, uri) + records, err := store.GetChangeStore().GetByURI(ctx, uri) if err != nil { return entity.BatchChanges{}, fmt.Errorf("failed to read change record for request %s uri=%s: %w", requestID, uri, err) } diff --git a/submitqueue/core/changeset/resolver_test.go b/submitqueue/core/changeset/resolver_test.go index 365443fd..67ec5589 100644 --- a/submitqueue/core/changeset/resolver_test.go +++ b/submitqueue/core/changeset/resolver_test.go @@ -25,9 +25,21 @@ import ( "github.com/uber/submitqueue/platform/base/change" "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" ) +// newTestResolver builds a Resolver over mock stores exposed through a mock +// storage factory that resolves every queue to the same aggregate. +func newTestResolver(ctrl *gomock.Controller, reqs storage.RequestStore, changes storage.ChangeStore) Resolver { + store := storagemock.NewMockStorage(ctrl) + store.EXPECT().GetRequestStore().Return(reqs).AnyTimes() + store.EXPECT().GetChangeStore().Return(changes).AnyTimes() + f := storagemock.NewMockFactory(ctrl) + f.EXPECT().For(gomock.Any()).Return(store, nil).AnyTimes() + return New(f) +} + func req(id string, uris ...string) entity.Request { return entity.Request{ID: id, Change: change.Change{URIs: uris}} } @@ -36,7 +48,7 @@ func TestResolverChanges(t *testing.T) { ctrl := gomock.NewController(t) reqs := storagemock.NewMockRequestStore(ctrl) changes := storagemock.NewMockChangeStore(ctrl) - r := New(reqs, changes) + r := newTestResolver(ctrl, reqs, changes) reqs.EXPECT().Get(gomock.Any(), "r2").Return(req("r2", "u2"), nil) reqs.EXPECT().Get(gomock.Any(), "r3").Return(req("r3", "u3"), nil) @@ -49,7 +61,7 @@ func TestResolverChanges(t *testing.T) { func TestResolverChangesEmpty(t *testing.T) { ctrl := gomock.NewController(t) - r := New(storagemock.NewMockRequestStore(ctrl), storagemock.NewMockChangeStore(ctrl)) + r := newTestResolver(ctrl, storagemock.NewMockRequestStore(ctrl), storagemock.NewMockChangeStore(ctrl)) got, err := r.ChangesForBatch(context.Background(), entity.Batch{ID: "q/batch/1"}) require.NoError(t, err) @@ -59,7 +71,7 @@ func TestResolverChangesEmpty(t *testing.T) { func TestResolverChangesRequestError(t *testing.T) { ctrl := gomock.NewController(t) reqs := storagemock.NewMockRequestStore(ctrl) - r := New(reqs, storagemock.NewMockChangeStore(ctrl)) + r := newTestResolver(ctrl, reqs, storagemock.NewMockChangeStore(ctrl)) sentinel := errors.New("not found") reqs.EXPECT().Get(gomock.Any(), "r1").Return(entity.Request{}, sentinel) @@ -72,7 +84,7 @@ func TestResolverDetailed(t *testing.T) { ctrl := gomock.NewController(t) reqs := storagemock.NewMockRequestStore(ctrl) changes := storagemock.NewMockChangeStore(ctrl) - r := New(reqs, changes) + r := newTestResolver(ctrl, reqs, changes) batch := entity.Batch{ID: "q/batch/1", Queue: "q", Contains: []string{"r1", "r2"}} reqs.EXPECT().Get(gomock.Any(), "r1").Return(req("r1", "u1"), nil) @@ -82,11 +94,11 @@ func TestResolverDetailed(t *testing.T) { d2 := entity.ChangeDetails{ChangedFiles: []entity.ChangedFile{{Path: "b.go", LinesAdded: 5}}} // GetByURI returns rows for every request that ever claimed the URI; the // resolver must pick the row owned by the requesting request. - changes.EXPECT().GetByURI(gomock.Any(), "q", "u1").Return([]entity.ChangeRecord{ + changes.EXPECT().GetByURI(gomock.Any(), "u1").Return([]entity.ChangeRecord{ {URI: "u1", RequestID: "other", Details: entity.ChangeDetails{}}, {URI: "u1", RequestID: "r1", Details: d1}, }, nil) - changes.EXPECT().GetByURI(gomock.Any(), "q", "u2").Return([]entity.ChangeRecord{ + changes.EXPECT().GetByURI(gomock.Any(), "u2").Return([]entity.ChangeRecord{ {URI: "u2", RequestID: "r2", Details: d2}, }, nil) @@ -103,11 +115,11 @@ func TestResolverDetailedChangeStoreError(t *testing.T) { ctrl := gomock.NewController(t) reqs := storagemock.NewMockRequestStore(ctrl) changes := storagemock.NewMockChangeStore(ctrl) - r := New(reqs, changes) + r := newTestResolver(ctrl, reqs, changes) sentinel := errors.New("read failed") reqs.EXPECT().Get(gomock.Any(), "r1").Return(req("r1", "u1"), nil) - changes.EXPECT().GetByURI(gomock.Any(), "q", "u1").Return(nil, sentinel) + changes.EXPECT().GetByURI(gomock.Any(), "u1").Return(nil, sentinel) _, err := r.DetailedForBatch(context.Background(), entity.Batch{ID: "q/batch/1", Queue: "q", Contains: []string{"r1"}}) require.ErrorIs(t, err, sentinel) diff --git a/submitqueue/core/request/materializer.go b/submitqueue/core/request/materializer.go index 909d1a9b..7cf247d3 100644 --- a/submitqueue/core/request/materializer.go +++ b/submitqueue/core/request/materializer.go @@ -27,25 +27,31 @@ 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. type Materializer struct { - store storage.Storage + logs storage.RequestLogStore + summaries storage.RequestSummaryStore + uris storage.RequestURIStore + stores storage.Factory } // NewMaterializer creates a request read-model materializer. -func NewMaterializer(store storage.Storage) *Materializer { - return &Materializer{store: store} +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} } // 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.store.GetRequestLogStore().Insert(ctx, log); err != nil { + if err := m.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.store.GetRequestSummaryStore().Get(ctx, log.RequestID) + summary, err := m.summaries.Get(ctx, log.RequestID) if err != nil { return fmt.Errorf("failed to get request summary request_id=%s: %w", log.RequestID, err) } @@ -60,7 +66,7 @@ func (m *Materializer) PersistLog(ctx context.Context, log entity.RequestLog) er updated.LastError = log.LastError updated.Metadata = cloneMetadata(log.Metadata) - if err := m.store.GetRequestSummaryStore().Update(ctx, updated, oldVersion, newVersion); err != nil { + if err := m.summaries.Update(ctx, updated, oldVersion, newVersion); err != nil { if errors.Is(err, storage.ErrVersionMismatch) { continue } @@ -81,13 +87,18 @@ func (m *Materializer) PersistLog(ctx context.Context, log entity.RequestLog) er // 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 { 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() for { - current, err := m.store.GetRequestQueueSummaryStore().Get(ctx, desired.Queue, desired.ReceivedAtMs, desired.RequestID) + current, err := queueSummaries.Get(ctx, desired.ReceivedAtMs, desired.RequestID) if errors.Is(err, storage.ErrNotFound) { if err := m.createURIMappings(ctx, authoritative); err != nil { return err } - if err := m.store.GetRequestQueueSummaryStore().Create(ctx, desired); err != nil { + if err := queueSummaries.Create(ctx, desired); err != nil { if errors.Is(err, storage.ErrAlreadyExists) { continue } @@ -105,7 +116,7 @@ func (m *Materializer) repairPublicProjections(ctx context.Context, authoritativ // Another materializer already projected a newer authoritative snapshot. return nil } - if err := m.store.GetRequestQueueSummaryStore().Update(ctx, desired, current.Version, desired.Version); err != nil { + if err := queueSummaries.Update(ctx, desired, current.Version, desired.Version); err != nil { if errors.Is(err, storage.ErrVersionMismatch) { continue } @@ -122,7 +133,7 @@ func (m *Materializer) createURIMappings(ctx context.Context, summary entity.Req ReceivedAtMs: summary.ReceivedAtMs, RequestID: summary.RequestID, } - if err := m.store.GetRequestURIStore().Create(ctx, mapping); err != nil && !errors.Is(err, storage.ErrAlreadyExists) { + if err := m.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 fd6c815c..a8be57ec 100644 --- a/submitqueue/core/request/materializer_test.go +++ b/submitqueue/core/request/materializer_test.go @@ -32,7 +32,7 @@ func TestMaterializer_PersistLog(t *testing.T) { log := entity.RequestLog{RequestID: "q/1", TimestampMs: 20, Status: entity.RequestStatusLanded, RequestVersion: 2, Metadata: map[string]string{}} t.Run("winning log updates both projections", func(t *testing.T) { ctrl := gomock.NewController(t) - store, summaryStore, queueStore, _, logStore := materializerStores(ctrl) + m, summaryStore, queueStore, _, logStore := materializerStores(ctrl) logStore.EXPECT().Insert(gomock.Any(), log).Return(nil) summaryStore.EXPECT().Get(gomock.Any(), "q/1").Return(base, nil) summaryStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).DoAndReturn(func(_ context.Context, updated entity.RequestSummary, _, _ int32) error { @@ -40,14 +40,14 @@ func TestMaterializer_PersistLog(t *testing.T) { assert.Equal(t, int32(2), updated.RequestVersion) return nil }) - queueStore.EXPECT().Get(gomock.Any(), "q", int64(10), "q/1").Return(queueSummaryFromSummary(base), nil) + queueStore.EXPECT().Get(gomock.Any(), int64(10), "q/1").Return(queueSummaryFromSummary(base), nil) queueStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(nil) - require.NoError(t, NewMaterializer(store).PersistLog(context.Background(), log)) + require.NoError(t, m.PersistLog(context.Background(), log)) }) t.Run("unversioned terminal status does not receive terminal precedence", func(t *testing.T) { ctrl := gomock.NewController(t) - store, summaryStore, queueStore, _, logStore := materializerStores(ctrl) + m, summaryStore, queueStore, _, logStore := materializerStores(ctrl) current := base current.Status = entity.RequestStatusLanded current.RequestVersion = 0 @@ -58,14 +58,14 @@ func TestMaterializer_PersistLog(t *testing.T) { assert.Equal(t, entity.RequestStatusProcessing, updated.Status) return nil }) - queueStore.EXPECT().Get(gomock.Any(), "q", int64(10), "q/1").Return(queueSummaryFromSummary(current), nil) + queueStore.EXPECT().Get(gomock.Any(), int64(10), "q/1").Return(queueSummaryFromSummary(current), nil) queueStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(nil) - require.NoError(t, NewMaterializer(store).PersistLog(context.Background(), incoming)) + require.NoError(t, m.PersistLog(context.Background(), incoming)) }) t.Run("CAS conflict reloads and repairs winner", func(t *testing.T) { ctrl := gomock.NewController(t) - store, summaryStore, queueStore, _, logStore := materializerStores(ctrl) + m, summaryStore, queueStore, _, logStore := materializerStores(ctrl) logStore.EXPECT().Insert(gomock.Any(), log).Return(nil) summaryStore.EXPECT().Get(gomock.Any(), "q/1").Return(base, nil) summaryStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(storage.ErrVersionMismatch) @@ -75,14 +75,14 @@ func TestMaterializer_PersistLog(t *testing.T) { advanced.StatusTimestampMs = 20 advanced.Version = 2 summaryStore.EXPECT().Get(gomock.Any(), "q/1").Return(advanced, nil) - queueStore.EXPECT().Get(gomock.Any(), "q", int64(10), "q/1").Return(queueSummaryFromSummary(base), nil) + queueStore.EXPECT().Get(gomock.Any(), int64(10), "q/1").Return(queueSummaryFromSummary(base), nil) queueStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(nil) - require.NoError(t, NewMaterializer(store).PersistLog(context.Background(), log)) + require.NoError(t, m.PersistLog(context.Background(), log)) }) t.Run("non-winning redelivery repairs stale queue projection", func(t *testing.T) { ctrl := gomock.NewController(t) - store, summaryStore, queueStore, _, logStore := materializerStores(ctrl) + m, summaryStore, queueStore, _, logStore := materializerStores(ctrl) logStore.EXPECT().Insert(gomock.Any(), log).Return(nil) advanced := base advanced.Status = entity.RequestStatusLanded @@ -90,14 +90,14 @@ func TestMaterializer_PersistLog(t *testing.T) { advanced.StatusTimestampMs = 20 advanced.Version = 2 summaryStore.EXPECT().Get(gomock.Any(), "q/1").Return(advanced, nil) - queueStore.EXPECT().Get(gomock.Any(), "q", int64(10), "q/1").Return(queueSummaryFromSummary(base), nil) + queueStore.EXPECT().Get(gomock.Any(), int64(10), "q/1").Return(queueSummaryFromSummary(base), nil) queueStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(nil) - require.NoError(t, NewMaterializer(store).PersistLog(context.Background(), log)) + require.NoError(t, m.PersistLog(context.Background(), log)) }) t.Run("first public event activates URI and queue projections", func(t *testing.T) { ctrl := gomock.NewController(t) - store, summaryStore, queueStore, uriStore, logStore := materializerStores(ctrl) + m, summaryStore, queueStore, uriStore, logStore := materializerStores(ctrl) logStore.EXPECT().Insert(gomock.Any(), log).Return(nil) summaryStore.EXPECT().Get(gomock.Any(), "q/1").Return(base, nil) summaryStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(nil) @@ -106,17 +106,17 @@ func TestMaterializer_PersistLog(t *testing.T) { activated.RequestVersion = 2 activated.StatusTimestampMs = 20 activated.Version = 2 - queueStore.EXPECT().Get(gomock.Any(), "q", int64(10), "q/1").Return(entity.RequestQueueSummary{}, storage.ErrNotFound) + 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) queueStore.EXPECT().Create(gomock.Any(), queueSummaryFromSummary(activated)).Return(nil) - require.NoError(t, NewMaterializer(store).PersistLog(context.Background(), log)) + require.NoError(t, m.PersistLog(context.Background(), log)) }) t.Run("retry after projection failure appends another audit row", func(t *testing.T) { ctrl := gomock.NewController(t) - store, summaryStore, queueStore, _, logStore := materializerStores(ctrl) - materializer := NewMaterializer(store) + m, summaryStore, queueStore, _, logStore := materializerStores(ctrl) + materializer := m advanced := base advanced.Status = entity.RequestStatusLanded advanced.RequestVersion = 2 @@ -124,8 +124,8 @@ func TestMaterializer_PersistLog(t *testing.T) { advanced.Version = 2 logStore.EXPECT().Insert(gomock.Any(), log).Return(nil).Times(2) summaryStore.EXPECT().Get(gomock.Any(), "q/1").Return(advanced, nil).Times(2) - queueStore.EXPECT().Get(gomock.Any(), "q", int64(10), "q/1").Return(entity.RequestQueueSummary{}, errors.New("queue store down")) - queueStore.EXPECT().Get(gomock.Any(), "q", int64(10), "q/1").Return(queueSummaryFromSummary(advanced), nil) + queueStore.EXPECT().Get(gomock.Any(), int64(10), "q/1").Return(entity.RequestQueueSummary{}, errors.New("queue store down")) + queueStore.EXPECT().Get(gomock.Any(), int64(10), "q/1").Return(queueSummaryFromSummary(advanced), nil) require.Error(t, materializer.PersistLog(context.Background(), log)) require.NoError(t, materializer.PersistLog(context.Background(), log)) @@ -133,15 +133,15 @@ func TestMaterializer_PersistLog(t *testing.T) { t.Run("missing authoritative summary fails", func(t *testing.T) { ctrl := gomock.NewController(t) - store, summaryStore, _, _, logStore := materializerStores(ctrl) + m, summaryStore, _, _, logStore := materializerStores(ctrl) logStore.EXPECT().Insert(gomock.Any(), log).Return(nil) summaryStore.EXPECT().Get(gomock.Any(), "q/1").Return(entity.RequestSummary{}, storage.ErrNotFound) - require.Error(t, NewMaterializer(store).PersistLog(context.Background(), log)) + require.Error(t, m.PersistLog(context.Background(), log)) }) t.Run("queue projection already ahead succeeds", func(t *testing.T) { ctrl := gomock.NewController(t) - store, summaryStore, queueStore, _, logStore := materializerStores(ctrl) + m, summaryStore, queueStore, _, logStore := materializerStores(ctrl) logStore.EXPECT().Insert(gomock.Any(), log).Return(nil) advanced := base advanced.Status = entity.RequestStatusLanded @@ -151,8 +151,8 @@ func TestMaterializer_PersistLog(t *testing.T) { summaryStore.EXPECT().Get(gomock.Any(), "q/1").Return(advanced, nil) queueAhead := queueSummaryFromSummary(advanced) queueAhead.Version = 3 - queueStore.EXPECT().Get(gomock.Any(), "q", int64(10), "q/1").Return(queueAhead, nil) - require.NoError(t, NewMaterializer(store).PersistLog(context.Background(), log)) + queueStore.EXPECT().Get(gomock.Any(), int64(10), "q/1").Return(queueAhead, nil) + require.NoError(t, m.PersistLog(context.Background(), log)) }) } @@ -249,17 +249,16 @@ func TestLogWins(t *testing.T) { } } -func materializerStores(ctrl *gomock.Controller) (*storagemock.MockStorage, *storagemock.MockRequestSummaryStore, *storagemock.MockRequestQueueSummaryStore, *storagemock.MockRequestURIStore, *storagemock.MockRequestLogStore) { - store := storagemock.NewMockStorage(ctrl) +func materializerStores(ctrl *gomock.Controller) (*Materializer, *storagemock.MockRequestSummaryStore, *storagemock.MockRequestQueueSummaryStore, *storagemock.MockRequestURIStore, *storagemock.MockRequestLogStore) { summaryStore := storagemock.NewMockRequestSummaryStore(ctrl) queueStore := storagemock.NewMockRequestQueueSummaryStore(ctrl) uriStore := storagemock.NewMockRequestURIStore(ctrl) logStore := storagemock.NewMockRequestLogStore(ctrl) - store.EXPECT().GetRequestSummaryStore().Return(summaryStore).AnyTimes() - store.EXPECT().GetRequestQueueSummaryStore().Return(queueStore).AnyTimes() - store.EXPECT().GetRequestURIStore().Return(uriStore).AnyTimes() - store.EXPECT().GetRequestLogStore().Return(logStore).AnyTimes() - return store, summaryStore, queueStore, uriStore, logStore + queueScoped := storagemock.NewMockStorage(ctrl) + queueScoped.EXPECT().GetRequestQueueSummaryStore().Return(queueStore).AnyTimes() + factory := storagemock.NewMockFactory(ctrl) + factory.EXPECT().For(gomock.Any()).Return(queueScoped, nil).AnyTimes() + return NewMaterializer(logStore, summaryStore, uriStore, factory), summaryStore, queueStore, uriStore, logStore } func testRequestSummary() entity.RequestSummary { diff --git a/submitqueue/extension/storage/README.md b/submitqueue/extension/storage/README.md index eeab72c5..21002ee3 100644 --- a/submitqueue/extension/storage/README.md +++ b/submitqueue/extension/storage/README.md @@ -2,6 +2,14 @@ Pluggable persistence interfaces for SubmitQueue entities (requests, batches, dependents, logs, etc.). Implementations live under `extension/storage//`. +## Queue-scoped resolution + +Storage follows the extension contract: the queue-scoped store aggregate is resolved per queue through a factory keyed by queue name, mirroring how every decision/action extension resolves its implementation. A resolved aggregate is bound to its queue — entity arguments whose queue disagrees with the binding are rejected, queue-keyed reads are implicitly scoped, and the host wiring decides which backend serves which queue (single shared backend by default). + +Three read-model stores are deliberately global rather than queue-scoped, because their lookups start from identifiers that arrive without queue context (a bare request ID or change URI at the status API): the request log, the request summary, and the change-URI mapping. They are injected individually as standalone seams, following the gateway's per-store injection. The queue registry (`queueconfig`) was never part of this aggregate and stays the registry the factory sits beside. + +The classification rule: a store is queue-scoped when every read path authoritatively holds the queue before the first read, and global when any read path begins from an identifier that arrives without queue context. Entity IDs are opaque — no reader may derive the queue from an ID prefix; the queue travels explicitly on payloads and requests. + ## Optimistic locking contract Entities that support concurrent mutation carry an `int32 Version` field. Updates are conditional on the version: the write only succeeds if the persisted version matches the caller's expected version. On mismatch, the implementation returns `storage.ErrVersionMismatch`, which is declared as a retryable infrastructure error so callers can return it without reclassifying it. diff --git a/submitqueue/extension/storage/change_store.go b/submitqueue/extension/storage/change_store.go index a95c460f..1eacc0fe 100644 --- a/submitqueue/extension/storage/change_store.go +++ b/submitqueue/extension/storage/change_store.go @@ -39,13 +39,13 @@ type ChangeStore interface { // cross-request overlap is detected by GetByURI, not by Create. Create(ctx context.Context, record entity.ChangeRecord) error - // GetByURI returns every ChangeRecord for the given (queue, uri). Multiple - // requests can have claimed the same URI over time, so the slice may have - // any number of entries; an empty slice means no claim has ever been - // recorded for this URI in this queue. + // GetByURI returns every ChangeRecord the bound queue holds for the given + // URI. Multiple requests can have claimed the same URI over time, so the + // slice may have any number of entries; an empty slice means no claim has + // ever been recorded for this URI in this queue. // // The store does not filter by request_id or by the owning request's // state — callers that want to skip self filter by RequestID, and callers // that want only live owners consult RequestStore for liveness. - GetByURI(ctx context.Context, queue string, uri string) ([]entity.ChangeRecord, error) + GetByURI(ctx context.Context, uri string) ([]entity.ChangeRecord, error) } diff --git a/submitqueue/extension/storage/mock/change_store_mock.go b/submitqueue/extension/storage/mock/change_store_mock.go index fa109d6c..520b33cc 100644 --- a/submitqueue/extension/storage/mock/change_store_mock.go +++ b/submitqueue/extension/storage/mock/change_store_mock.go @@ -56,16 +56,16 @@ func (mr *MockChangeStoreMockRecorder) Create(ctx, record any) *gomock.Call { } // GetByURI mocks base method. -func (m *MockChangeStore) GetByURI(ctx context.Context, queue, uri string) ([]entity.ChangeRecord, error) { +func (m *MockChangeStore) GetByURI(ctx context.Context, uri string) ([]entity.ChangeRecord, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetByURI", ctx, queue, uri) + ret := m.ctrl.Call(m, "GetByURI", ctx, uri) ret0, _ := ret[0].([]entity.ChangeRecord) ret1, _ := ret[1].(error) return ret0, ret1 } // GetByURI indicates an expected call of GetByURI. -func (mr *MockChangeStoreMockRecorder) GetByURI(ctx, queue, uri any) *gomock.Call { +func (mr *MockChangeStoreMockRecorder) GetByURI(ctx, uri any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetByURI", reflect.TypeOf((*MockChangeStore)(nil).GetByURI), ctx, queue, uri) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetByURI", reflect.TypeOf((*MockChangeStore)(nil).GetByURI), ctx, uri) } diff --git a/submitqueue/extension/storage/mock/queue_batch_state_store_mock.go b/submitqueue/extension/storage/mock/queue_batch_state_store_mock.go index 2b0c2e0f..373c412d 100644 --- a/submitqueue/extension/storage/mock/queue_batch_state_store_mock.go +++ b/submitqueue/extension/storage/mock/queue_batch_state_store_mock.go @@ -42,32 +42,32 @@ func (m *MockQueueBatchStateStore) EXPECT() *MockQueueBatchStateStoreMockRecorde } // Delete mocks base method. -func (m *MockQueueBatchStateStore) Delete(ctx context.Context, queue string, state entity.BatchState, batchID string) error { +func (m *MockQueueBatchStateStore) Delete(ctx context.Context, state entity.BatchState, batchID string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Delete", ctx, queue, state, batchID) + ret := m.ctrl.Call(m, "Delete", ctx, state, batchID) ret0, _ := ret[0].(error) return ret0 } // Delete indicates an expected call of Delete. -func (mr *MockQueueBatchStateStoreMockRecorder) Delete(ctx, queue, state, batchID any) *gomock.Call { +func (mr *MockQueueBatchStateStoreMockRecorder) Delete(ctx, state, batchID any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockQueueBatchStateStore)(nil).Delete), ctx, queue, state, batchID) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockQueueBatchStateStore)(nil).Delete), ctx, state, batchID) } // List mocks base method. -func (m *MockQueueBatchStateStore) List(ctx context.Context, queue string, state entity.BatchState) ([]entity.QueueBatchState, error) { +func (m *MockQueueBatchStateStore) List(ctx context.Context, state entity.BatchState) ([]entity.QueueBatchState, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "List", ctx, queue, state) + ret := m.ctrl.Call(m, "List", ctx, state) ret0, _ := ret[0].([]entity.QueueBatchState) ret1, _ := ret[1].(error) return ret0, ret1 } // List indicates an expected call of List. -func (mr *MockQueueBatchStateStoreMockRecorder) List(ctx, queue, state any) *gomock.Call { +func (mr *MockQueueBatchStateStoreMockRecorder) List(ctx, state any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "List", reflect.TypeOf((*MockQueueBatchStateStore)(nil).List), ctx, queue, state) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "List", reflect.TypeOf((*MockQueueBatchStateStore)(nil).List), ctx, state) } // Put mocks base method. diff --git a/submitqueue/extension/storage/mock/request_queue_summary_store_mock.go b/submitqueue/extension/storage/mock/request_queue_summary_store_mock.go index 5d7698af..7186e4a7 100644 --- a/submitqueue/extension/storage/mock/request_queue_summary_store_mock.go +++ b/submitqueue/extension/storage/mock/request_queue_summary_store_mock.go @@ -57,18 +57,18 @@ func (mr *MockRequestQueueSummaryStoreMockRecorder) Create(ctx, summary any) *go } // Get mocks base method. -func (m *MockRequestQueueSummaryStore) Get(ctx context.Context, queue string, receivedAtMs int64, requestID string) (entity.RequestQueueSummary, error) { +func (m *MockRequestQueueSummaryStore) Get(ctx context.Context, receivedAtMs int64, requestID string) (entity.RequestQueueSummary, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Get", ctx, queue, receivedAtMs, requestID) + ret := m.ctrl.Call(m, "Get", ctx, receivedAtMs, requestID) ret0, _ := ret[0].(entity.RequestQueueSummary) ret1, _ := ret[1].(error) return ret0, ret1 } // Get indicates an expected call of Get. -func (mr *MockRequestQueueSummaryStoreMockRecorder) Get(ctx, queue, receivedAtMs, requestID any) *gomock.Call { +func (mr *MockRequestQueueSummaryStoreMockRecorder) Get(ctx, receivedAtMs, requestID any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockRequestQueueSummaryStore)(nil).Get), ctx, queue, receivedAtMs, requestID) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockRequestQueueSummaryStore)(nil).Get), ctx, receivedAtMs, requestID) } // List mocks base method. diff --git a/submitqueue/extension/storage/mock/storage_mock.go b/submitqueue/extension/storage/mock/storage_mock.go index 181c4cb6..d1fbc6b3 100644 --- a/submitqueue/extension/storage/mock/storage_mock.go +++ b/submitqueue/extension/storage/mock/storage_mock.go @@ -16,6 +16,45 @@ import ( 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 storage.Config) (storage.Storage, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "For", config) + ret0, _ := ret[0].(storage.Storage) + 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) +} + // MockStorage is a mock of Storage interface. type MockStorage struct { ctrl *gomock.Controller @@ -40,20 +79,6 @@ func (m *MockStorage) EXPECT() *MockStorageMockRecorder { return m.recorder } -// Close mocks base method. -func (m *MockStorage) Close() error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Close") - ret0, _ := ret[0].(error) - return ret0 -} - -// Close indicates an expected call of Close. -func (mr *MockStorageMockRecorder) Close() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Close", reflect.TypeOf((*MockStorage)(nil).Close)) -} - // GetBatchDependentStore mocks base method. func (m *MockStorage) GetBatchDependentStore() storage.BatchDependentStore { m.ctrl.T.Helper() @@ -138,20 +163,6 @@ 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() @@ -179,31 +190,3 @@ func (mr *MockStorageMockRecorder) GetRequestStore() *gomock.Call { mr.mock.ctrl.T.Helper() 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)) -} diff --git a/submitqueue/extension/storage/mysql/batch_dependent_store.go b/submitqueue/extension/storage/mysql/batch_dependent_store.go index 3577fc39..8e424079 100644 --- a/submitqueue/extension/storage/mysql/batch_dependent_store.go +++ b/submitqueue/extension/storage/mysql/batch_dependent_store.go @@ -32,11 +32,14 @@ import ( type batchDependentStore 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 } // NewBatchDependentStore creates a new MySQL-backed BatchDependentStore. -func NewBatchDependentStore(db *sql.DB, scope tally.Scope) storage.BatchDependentStore { - return &batchDependentStore{db: db, scope: scope} +func NewBatchDependentStore(db *sql.DB, scope tally.Scope, queue string) storage.BatchDependentStore { + return &batchDependentStore{db: db, scope: scope, queue: queue} } // Get retrieves the batch dependent by batch ID. Returns ErrNotFound if the batch dependent is not found. diff --git a/submitqueue/extension/storage/mysql/batch_dependent_store_test.go b/submitqueue/extension/storage/mysql/batch_dependent_store_test.go index cef3c49a..194dcebd 100644 --- a/submitqueue/extension/storage/mysql/batch_dependent_store_test.go +++ b/submitqueue/extension/storage/mysql/batch_dependent_store_test.go @@ -36,7 +36,7 @@ func setupBatchDependentStoreTest(t *testing.T) (*sql.DB, sqlmock.Sqlmock, stora db, mock, err := sqlmock.New() require.NoError(t, err) - store := NewBatchDependentStore(db, testMetrics()) + store := NewBatchDependentStore(db, testMetrics(), "monorepo") return db, mock, store } diff --git a/submitqueue/extension/storage/mysql/batch_store.go b/submitqueue/extension/storage/mysql/batch_store.go index dbfa51c9..b3ad7fb2 100644 --- a/submitqueue/extension/storage/mysql/batch_store.go +++ b/submitqueue/extension/storage/mysql/batch_store.go @@ -32,11 +32,14 @@ import ( type batchStore 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 } // NewBatchStore creates a new MySQL-backed BatchStore. -func NewBatchStore(db *sql.DB, scope tally.Scope) storage.BatchStore { - return &batchStore{db: db, scope: scope} +func NewBatchStore(db *sql.DB, scope tally.Scope, queue string) storage.BatchStore { + return &batchStore{db: db, scope: scope, queue: queue} } // Get retrieves a batch by ID. Returns ErrNotFound if the batch is not found. @@ -76,6 +79,10 @@ func (s *batchStore) Create(ctx context.Context, batch entity.Batch) (retErr err op := metrics.Begin(s.scope, "create", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + if batch.Queue != s.queue { + return fmt.Errorf("batch %s queue %q does not match the store's bound queue %q", batch.ID, batch.Queue, s.queue) + } + containsJSON, err := json.Marshal(batch.Contains) if err != nil { return fmt.Errorf("failed to marshal contains=%v id=%s for Create batch entity: %w", batch.Contains, batch.ID, err) @@ -108,6 +115,10 @@ func (s *batchStore) Update(ctx context.Context, batch entity.Batch, oldVersion, op := metrics.Begin(s.scope, "update_state", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + if batch.Queue != s.queue { + return fmt.Errorf("batch %s queue %q does not match the store's bound queue %q", batch.ID, batch.Queue, s.queue) + } + containsJSON, err := json.Marshal(batch.Contains) if err != nil { return fmt.Errorf("failed to marshal contains=%v id=%s for Update batch entity: %w", batch.Contains, batch.ID, err) diff --git a/submitqueue/extension/storage/mysql/batch_store_test.go b/submitqueue/extension/storage/mysql/batch_store_test.go index 34c833dd..96182f8e 100644 --- a/submitqueue/extension/storage/mysql/batch_store_test.go +++ b/submitqueue/extension/storage/mysql/batch_store_test.go @@ -36,7 +36,7 @@ func setupBatchStoreTest(t *testing.T) (*sql.DB, sqlmock.Sqlmock, storage.BatchS db, mock, err := sqlmock.New() require.NoError(t, err) - store := NewBatchStore(db, testMetrics()) + store := NewBatchStore(db, testMetrics(), "monorepo") return db, mock, store } @@ -202,7 +202,7 @@ func TestBatchStore_Update(t *testing.T) { const oldVersion, newVersion = int32(1), int32(2) batch := entity.Batch{ ID: "monorepo/batch/1", - Queue: "monorepo-updated", + Queue: "monorepo", Contains: []string{"monorepo/3", "monorepo/4"}, Dependencies: []string{"monorepo/batch/1", "monorepo/batch/2"}, State: entity.BatchStateMerging, diff --git a/submitqueue/extension/storage/mysql/build_store.go b/submitqueue/extension/storage/mysql/build_store.go index f1795ffb..65a6304f 100644 --- a/submitqueue/extension/storage/mysql/build_store.go +++ b/submitqueue/extension/storage/mysql/build_store.go @@ -31,11 +31,14 @@ import ( type buildStore 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 } // NewBuildStore creates a new MySQL-backed BuildStore. -func NewBuildStore(db *sql.DB, scope tally.Scope) storage.BuildStore { - return &buildStore{db: db, scope: scope} +func NewBuildStore(db *sql.DB, scope tally.Scope, queue string) storage.BuildStore { + return &buildStore{db: db, scope: scope, queue: queue} } // Get retrieves a build by ID. Returns ErrNotFound if the build is not found. diff --git a/submitqueue/extension/storage/mysql/build_store_test.go b/submitqueue/extension/storage/mysql/build_store_test.go index 9d350db8..b1cbe163 100644 --- a/submitqueue/extension/storage/mysql/build_store_test.go +++ b/submitqueue/extension/storage/mysql/build_store_test.go @@ -35,7 +35,7 @@ func setupBuildStoreTest(t *testing.T) (*sql.DB, sqlmock.Sqlmock, storage.BuildS db, mock, err := sqlmock.New() require.NoError(t, err) - store := NewBuildStore(db, testMetrics()) + store := NewBuildStore(db, testMetrics(), "monorepo") return db, mock, store } diff --git a/submitqueue/extension/storage/mysql/change_store.go b/submitqueue/extension/storage/mysql/change_store.go index 1167fb00..72921e35 100644 --- a/submitqueue/extension/storage/mysql/change_store.go +++ b/submitqueue/extension/storage/mysql/change_store.go @@ -30,11 +30,14 @@ import ( type changeStore 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 } // NewChangeStore creates a new MySQL-backed ChangeStore. -func NewChangeStore(db *sql.DB, scope tally.Scope) storage.ChangeStore { - return &changeStore{db: db, scope: scope} +func NewChangeStore(db *sql.DB, scope tally.Scope, queue string) storage.ChangeStore { + return &changeStore{db: db, scope: scope, queue: queue} } // Create inserts a single ChangeRecord. A primary-key conflict on @@ -44,6 +47,10 @@ func (s *changeStore) Create(ctx context.Context, record entity.ChangeRecord) (r op := metrics.Begin(s.scope, "create", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + if record.Queue != s.queue { + return fmt.Errorf("change record uri=%s request_id=%s queue %q does not match the store's bound queue %q", record.URI, record.RequestID, record.Queue, s.queue) + } + detailsJSON, err := marshalDetails(record.Details) if err != nil { return fmt.Errorf("failed to marshal details for change record uri=%s request_id=%s: %w", record.URI, record.RequestID, err) @@ -58,12 +65,14 @@ func (s *changeStore) Create(ctx context.Context, record entity.ChangeRecord) (r return nil } -// GetByURI returns every ChangeRecord for (queue, uri). queue leads the WHERE -// clause to align with the (queue, uri, request_id) PK so this is a PK-prefix scan. -func (s *changeStore) GetByURI(ctx context.Context, queue string, uri string) (ret []entity.ChangeRecord, retErr error) { +// GetByURI returns every ChangeRecord for the bound queue's uri. queue leads the +// WHERE clause to align with the (queue, uri, request_id) PK so this is a +// PK-prefix scan. +func (s *changeStore) GetByURI(ctx context.Context, uri string) (ret []entity.ChangeRecord, retErr error) { op := metrics.Begin(s.scope, "get_by_uri", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + queue := s.queue const query = "SELECT uri, request_id, queue, details, created_at, updated_at, version FROM `change` WHERE queue = ? AND uri = ?" rows, err := s.db.QueryContext(ctx, query, queue, uri) if err != nil { diff --git a/submitqueue/extension/storage/mysql/change_store_test.go b/submitqueue/extension/storage/mysql/change_store_test.go index db80fddc..cf686a17 100644 --- a/submitqueue/extension/storage/mysql/change_store_test.go +++ b/submitqueue/extension/storage/mysql/change_store_test.go @@ -34,7 +34,7 @@ func setupChangeStoreTest(t *testing.T) (*sql.DB, sqlmock.Sqlmock, storage.Chang db, mock, err := sqlmock.New() require.NoError(t, err) - store := NewChangeStore(db, testMetrics()) + store := NewChangeStore(db, testMetrics(), "monorepo") return db, mock, store } @@ -165,7 +165,7 @@ func TestChangeStore_GetByURI(t *testing.T) { tt.setup(mock) - got, err := store.GetByURI(context.Background(), record.Queue, record.URI) + got, err := store.GetByURI(context.Background(), record.URI) if tt.wantErr { require.Error(t, err) } else { diff --git a/submitqueue/extension/storage/mysql/queue_batch_state_store.go b/submitqueue/extension/storage/mysql/queue_batch_state_store.go index 1e43c0d1..11302a1f 100644 --- a/submitqueue/extension/storage/mysql/queue_batch_state_store.go +++ b/submitqueue/extension/storage/mysql/queue_batch_state_store.go @@ -29,19 +29,24 @@ import ( type queueBatchStateStore 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 } // NewQueueBatchStateStore creates a new MySQL-backed QueueBatchStateStore. -func NewQueueBatchStateStore(db *sql.DB, scope tally.Scope) storage.QueueBatchStateStore { - return &queueBatchStateStore{db: db, scope: scope} +func NewQueueBatchStateStore(db *sql.DB, scope tally.Scope, queue string) storage.QueueBatchStateStore { + return &queueBatchStateStore{db: db, scope: scope, queue: queue} } -// List returns every record filed under (queue, state). The WHERE clause is a -// prefix of the (queue, state, batch_id) PK, so this is a PK-prefix scan. -func (s *queueBatchStateStore) List(ctx context.Context, queue string, state entity.BatchState) (ret []entity.QueueBatchState, retErr error) { +// List returns every record filed under the bound queue's state bucket. The +// WHERE clause is a prefix of the (queue, state, batch_id) PK, so this is a +// PK-prefix scan. +func (s *queueBatchStateStore) List(ctx context.Context, state entity.BatchState) (ret []entity.QueueBatchState, retErr error) { op := metrics.Begin(s.scope, "list", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + queue := s.queue const query = "SELECT queue, state, batch_id FROM queue_batch_state WHERE queue = ? AND state = ?" rows, err := s.db.QueryContext(ctx, query, queue, string(state)) if err != nil { @@ -70,6 +75,10 @@ func (s *queueBatchStateStore) Put(ctx context.Context, record entity.QueueBatch op := metrics.Begin(s.scope, "put", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + if record.Queue != s.queue { + return fmt.Errorf("queue batch state record batch_id=%s queue %q does not match the store's bound queue %q", record.BatchID, record.Queue, s.queue) + } + const query = "INSERT IGNORE INTO queue_batch_state (queue, state, batch_id) VALUES (?, ?, ?)" if _, err := s.db.ExecContext(ctx, query, record.Queue, string(record.State), record.BatchID); err != nil { return fmt.Errorf("failed to put queue batch state record queue=%s state=%s batch_id=%s: %w", record.Queue, record.State, record.BatchID, err) @@ -77,12 +86,14 @@ func (s *queueBatchStateStore) Put(ctx context.Context, record entity.QueueBatch return nil } -// Delete removes the record identified by (queue, state, batchID). Deleting an -// absent record is a no-op success — rows-affected is intentionally not checked. -func (s *queueBatchStateStore) Delete(ctx context.Context, queue string, state entity.BatchState, batchID string) (retErr error) { +// Delete removes the bound queue's record identified by (state, batchID). +// Deleting an absent record is a no-op success — rows-affected is intentionally +// not checked. +func (s *queueBatchStateStore) Delete(ctx context.Context, state entity.BatchState, batchID string) (retErr error) { op := metrics.Begin(s.scope, "delete", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + queue := s.queue const query = "DELETE FROM queue_batch_state WHERE queue = ? AND state = ? AND batch_id = ?" if _, err := s.db.ExecContext(ctx, query, queue, string(state), batchID); err != nil { return fmt.Errorf("failed to delete queue batch state record queue=%s state=%s batch_id=%s: %w", queue, state, batchID, err) diff --git a/submitqueue/extension/storage/mysql/queue_batch_state_store_test.go b/submitqueue/extension/storage/mysql/queue_batch_state_store_test.go index 652c15f3..6bca7bd1 100644 --- a/submitqueue/extension/storage/mysql/queue_batch_state_store_test.go +++ b/submitqueue/extension/storage/mysql/queue_batch_state_store_test.go @@ -34,7 +34,7 @@ func setupQueueBatchStateStoreTest(t *testing.T) (*sql.DB, sqlmock.Sqlmock, stor db, mock, err := sqlmock.New() require.NoError(t, err) - return db, mock, NewQueueBatchStateStore(db, testMetrics()) + return db, mock, NewQueueBatchStateStore(db, testMetrics(), "monorepo") } func TestQueueBatchStateStore_List(t *testing.T) { @@ -99,7 +99,7 @@ func TestQueueBatchStateStore_List(t *testing.T) { defer db.Close() tt.setup(mock) - got, err := store.List(context.Background(), record1.Queue, record1.State) + got, err := store.List(context.Background(), record1.State) if tt.errMsg != "" { assert.ErrorContains(t, err, tt.errMsg) } else { @@ -203,7 +203,7 @@ func TestQueueBatchStateStore_Delete(t *testing.T) { defer db.Close() tt.setup(mock) - err := store.Delete(context.Background(), record.Queue, record.State, record.BatchID) + err := store.Delete(context.Background(), record.State, record.BatchID) if tt.errMsg != "" { assert.ErrorContains(t, err, tt.errMsg) } else { diff --git a/submitqueue/extension/storage/mysql/request_batch_store.go b/submitqueue/extension/storage/mysql/request_batch_store.go index 3986418f..4e4bb5cb 100644 --- a/submitqueue/extension/storage/mysql/request_batch_store.go +++ b/submitqueue/extension/storage/mysql/request_batch_store.go @@ -31,11 +31,14 @@ import ( type requestBatchStore 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 } // NewRequestBatchStore creates a MySQL-backed RequestBatchStore. -func NewRequestBatchStore(db *sql.DB, scope tally.Scope) storage.RequestBatchStore { - return &requestBatchStore{db: db, scope: scope} +func NewRequestBatchStore(db *sql.DB, scope tally.Scope, queue string) storage.RequestBatchStore { + return &requestBatchStore{db: db, scope: scope, queue: queue} } func (s *requestBatchStore) GetByRequestID(ctx context.Context, requestID string) (ret []entity.RequestBatch, retErr error) { diff --git a/submitqueue/extension/storage/mysql/request_batch_store_test.go b/submitqueue/extension/storage/mysql/request_batch_store_test.go index 3c2c48d5..d2c2f6ca 100644 --- a/submitqueue/extension/storage/mysql/request_batch_store_test.go +++ b/submitqueue/extension/storage/mysql/request_batch_store_test.go @@ -35,7 +35,7 @@ func setupRequestBatchStoreTest(t *testing.T) (*sql.DB, sqlmock.Sqlmock, storage db, mock, err := sqlmock.New() require.NoError(t, err) - return db, mock, NewRequestBatchStore(db, testMetrics()) + return db, mock, NewRequestBatchStore(db, testMetrics(), "monorepo") } func TestRequestBatchStore_GetByRequestID(t *testing.T) { diff --git a/submitqueue/extension/storage/mysql/request_queue_summary_store.go b/submitqueue/extension/storage/mysql/request_queue_summary_store.go index 2b4f537e..f00c49d6 100644 --- a/submitqueue/extension/storage/mysql/request_queue_summary_store.go +++ b/submitqueue/extension/storage/mysql/request_queue_summary_store.go @@ -31,17 +31,24 @@ import ( type requestQueueSummaryStore 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 } // NewRequestQueueSummaryStore creates a MySQL-backed RequestQueueSummaryStore. -func NewRequestQueueSummaryStore(db *sql.DB, scope tally.Scope) storage.RequestQueueSummaryStore { - return &requestQueueSummaryStore{db: db, scope: scope} +func NewRequestQueueSummaryStore(db *sql.DB, scope tally.Scope, queue string) storage.RequestQueueSummaryStore { + return &requestQueueSummaryStore{db: db, scope: scope, queue: queue} } func (s *requestQueueSummaryStore) Create(ctx context.Context, summary entity.RequestQueueSummary) (retErr error) { op := metrics.Begin(s.scope, "create", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + if summary.Queue != s.queue { + return fmt.Errorf("queue 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 queue summary metadata request_id=%s: %w", summary.RequestID, err) @@ -64,10 +71,12 @@ func (s *requestQueueSummaryStore) Create(ctx context.Context, summary entity.Re return nil } -func (s *requestQueueSummaryStore) Get(ctx context.Context, queue string, receivedAtMs int64, requestID string) (ret entity.RequestQueueSummary, retErr error) { +func (s *requestQueueSummaryStore) Get(ctx context.Context, receivedAtMs int64, requestID string) (ret entity.RequestQueueSummary, retErr error) { op := metrics.Begin(s.scope, "get", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + queue := s.queue + var changeURIsJSON []byte var metadataJSON []byte err := s.db.QueryRowContext(ctx, ` @@ -92,6 +101,10 @@ func (s *requestQueueSummaryStore) Update(ctx context.Context, summary entity.Re op := metrics.Begin(s.scope, "update", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + if summary.Queue != s.queue { + return fmt.Errorf("queue 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 queue summary request_id=%s: %w", summary.RequestID, err) @@ -125,7 +138,7 @@ func (s *requestQueueSummaryStore) List(ctx context.Context, query storage.Reque version, last_error, metadata FROM request_summary_by_queue WHERE queue = ? AND received_at_ms >= ? AND received_at_ms < ?` - args := []any{query.Queue, query.ReceivedAtOrAfterMs, query.ReceivedBeforeMs} + args := []any{s.queue, query.ReceivedAtOrAfterMs, query.ReceivedBeforeMs} if query.HasCursor { statement += " AND (received_at_ms < ? OR (received_at_ms = ? AND request_id < ?))" args = append(args, query.Cursor.ReceivedAtMs, query.Cursor.ReceivedAtMs, query.Cursor.RequestID) @@ -135,7 +148,7 @@ func (s *requestQueueSummaryStore) List(ctx context.Context, query storage.Reque rows, err := s.db.QueryContext(ctx, statement, args...) if err != nil { - return nil, fmt.Errorf("failed to list queue summaries queue=%s: %w", query.Queue, err) + return nil, fmt.Errorf("failed to list queue summaries queue=%s: %w", s.queue, err) } defer rows.Close() @@ -145,7 +158,7 @@ func (s *requestQueueSummaryStore) List(ctx context.Context, query storage.Reque var changeURIsJSON []byte var metadataJSON []byte if err := rows.Scan(&summary.Queue, &summary.ReceivedAtMs, &summary.RequestID, &changeURIsJSON, &summary.Status, &summary.Version, &summary.LastError, &metadataJSON); err != nil { - return nil, fmt.Errorf("failed to scan queue summary queue=%s: %w", query.Queue, err) + return nil, fmt.Errorf("failed to scan queue summary queue=%s: %w", s.queue, err) } if err := unmarshalSummaryJSON(changeURIsJSON, metadataJSON, &summary.ChangeURIs, &summary.Metadata); err != nil { return nil, fmt.Errorf("failed to decode queue summary request_id=%s: %w", summary.RequestID, err) @@ -153,7 +166,7 @@ func (s *requestQueueSummaryStore) List(ctx context.Context, query storage.Reque results = append(results, summary) } if err := rows.Err(); err != nil { - return nil, fmt.Errorf("failed to iterate queue summaries queue=%s: %w", query.Queue, err) + return nil, fmt.Errorf("failed to iterate queue summaries queue=%s: %w", s.queue, err) } return results, nil } diff --git a/submitqueue/extension/storage/mysql/request_queue_summary_store_test.go b/submitqueue/extension/storage/mysql/request_queue_summary_store_test.go index 8643030c..8627748f 100644 --- a/submitqueue/extension/storage/mysql/request_queue_summary_store_test.go +++ b/submitqueue/extension/storage/mysql/request_queue_summary_store_test.go @@ -35,7 +35,7 @@ func setupRequestQueueSummaryStoreTest(t *testing.T) (*sql.DB, sqlmock.Sqlmock, db, mock, err := sqlmock.New() require.NoError(t, err) - store := NewRequestQueueSummaryStore(db, testMetrics()) + store := NewRequestQueueSummaryStore(db, testMetrics(), "monorepo") return db, mock, store } @@ -166,7 +166,7 @@ func TestRequestQueueSummaryStore_Get(t *testing.T) { requestID = "missing" } - got, err := store.Get(context.Background(), "monorepo", 1000, requestID) + got, err := store.Get(context.Background(), 1000, requestID) if tt.wantErr { require.Error(t, err) if tt.wantErrIs != nil { @@ -324,7 +324,6 @@ func TestRequestQueueSummaryStore_List(t *testing.T) { { name: "without cursor", query: storage.RequestQueueSummaryQuery{ - Queue: "monorepo", ReceivedAtOrAfterMs: 0, ReceivedBeforeMs: 2000, Limit: 10, @@ -350,7 +349,6 @@ func TestRequestQueueSummaryStore_List(t *testing.T) { { name: "with cursor", query: storage.RequestQueueSummaryQuery{ - Queue: "monorepo", ReceivedAtOrAfterMs: 0, ReceivedBeforeMs: 2000, Limit: 10, @@ -371,7 +369,6 @@ func TestRequestQueueSummaryStore_List(t *testing.T) { { name: "query error", query: storage.RequestQueueSummaryQuery{ - Queue: "monorepo", ReceivedAtOrAfterMs: 0, ReceivedBeforeMs: 2000, Limit: 10, diff --git a/submitqueue/extension/storage/mysql/request_store.go b/submitqueue/extension/storage/mysql/request_store.go index 01a4487f..605dc29f 100644 --- a/submitqueue/extension/storage/mysql/request_store.go +++ b/submitqueue/extension/storage/mysql/request_store.go @@ -32,11 +32,14 @@ import ( type requestStore 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 } // NewRequestStore creates a new MySQL-backed RequestStore. -func NewRequestStore(db *sql.DB, scope tally.Scope) storage.RequestStore { - return &requestStore{db: db, scope: scope} +func NewRequestStore(db *sql.DB, scope tally.Scope, queue string) storage.RequestStore { + return &requestStore{db: db, scope: scope, queue: queue} } // Get retrieves a land request by ID. Returns ErrNotFound if the request is not found. @@ -72,6 +75,10 @@ func (r *requestStore) Create(ctx context.Context, request entity.Request) (retE op := metrics.Begin(r.scope, "create", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + if request.Queue != r.queue { + return fmt.Errorf("request %s queue %q does not match the store's bound queue %q", request.ID, request.Queue, r.queue) + } + // Marshal the change URIs to JSON changeURIsJSON, err := json.Marshal(request.Change.URIs) if err != nil { @@ -99,6 +106,10 @@ func (r *requestStore) Update(ctx context.Context, request entity.Request, oldVe op := metrics.Begin(r.scope, "update_state", metrics.StorageLatencyBuckets) defer func() { op.Complete(retErr) }() + if request.Queue != r.queue { + return fmt.Errorf("request %s queue %q does not match the store's bound queue %q", request.ID, request.Queue, r.queue) + } + changeURIsJSON, err := json.Marshal(request.Change.URIs) if err != nil { return fmt.Errorf("failed to marshal change URIs for request id=%s: %w", request.ID, err) diff --git a/submitqueue/extension/storage/mysql/request_store_test.go b/submitqueue/extension/storage/mysql/request_store_test.go index 1c37bcb1..7f4ef711 100644 --- a/submitqueue/extension/storage/mysql/request_store_test.go +++ b/submitqueue/extension/storage/mysql/request_store_test.go @@ -38,7 +38,7 @@ func setupRequestStoreTest(t *testing.T) (*sql.DB, sqlmock.Sqlmock, storage.Requ db, mock, err := sqlmock.New() require.NoError(t, err) - store := NewRequestStore(db, testMetrics()) + store := NewRequestStore(db, testMetrics(), "monorepo") return db, mock, store } @@ -202,7 +202,7 @@ func TestRequestStore_Update(t *testing.T) { const oldVersion, newVersion = int32(1), int32(2) request := entity.Request{ ID: "monorepo/1", - Queue: "monorepo-updated", + Queue: "monorepo", Change: change.Change{URIs: []string{"github://github.example.com/uber/submitqueue/pull/456/cafebabe"}}, LandStrategy: mergestrategy.MergeStrategySquashRebase, State: entity.RequestStateValidated, diff --git a/submitqueue/extension/storage/mysql/storage.go b/submitqueue/extension/storage/mysql/storage.go index e6f174bd..249c6a0c 100644 --- a/submitqueue/extension/storage/mysql/storage.go +++ b/submitqueue/extension/storage/mysql/storage.go @@ -16,6 +16,7 @@ package mysql import ( "database/sql" + "fmt" _ "github.com/go-sql-driver/mysql" "github.com/uber-go/tally" @@ -27,8 +28,71 @@ import ( // It requires a unique index on the table to be raised. const mysqlErrDuplicateEntry = 1062 -type mysqlStorage struct { - db *sql.DB +// 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. +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 +} + +// For returns the queue-scoped store aggregate bound to queueName over the +// shared pool. Every store the aggregate hands back reads and writes only that +// queue's records. +func (s *Storage) For(queueName string) (storage.Storage, error) { + if queueName == "" { + return nil, fmt.Errorf("queue name must not be empty") + } + return &boundStorage{ + requestStore: NewRequestStore(s.db, s.scope.SubScope("request_store"), queueName), + requestBatchStore: NewRequestBatchStore(s.db, s.scope.SubScope("request_batch_store"), queueName), + changeStore: NewChangeStore(s.db, s.scope.SubScope("change_store"), queueName), + batchStore: NewBatchStore(s.db, s.scope.SubScope("batch_store"), queueName), + 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), + requestQueueStore: NewRequestQueueSummaryStore(s.db, s.scope.SubScope("request_queue_summary_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() +} + +// boundStorage is the queue-scoped store aggregate returned by For. +type boundStorage struct { requestStore storage.RequestStore requestBatchStore storage.RequestBatchStore changeStore storage.ChangeStore @@ -36,86 +100,48 @@ type mysqlStorage struct { batchDependentStore storage.BatchDependentStore queueBatchStateStore storage.QueueBatchStateStore buildStore storage.BuildStore - requestLogStore storage.RequestLogStore - requestSummaryStore storage.RequestSummaryStore requestQueueStore storage.RequestQueueSummaryStore - requestURIStore storage.RequestURIStore -} - -// NewStorage creates a new MySQL storage. -func NewStorage(db *sql.DB, scope tally.Scope) (storage.Storage, error) { - return &mysqlStorage{ - db: db, - requestStore: NewRequestStore(db, scope.SubScope("request_store")), - requestBatchStore: NewRequestBatchStore(db, scope.SubScope("request_batch_store")), - changeStore: NewChangeStore(db, scope.SubScope("change_store")), - batchStore: NewBatchStore(db, scope.SubScope("batch_store")), - batchDependentStore: NewBatchDependentStore(db, scope.SubScope("batch_dependent_store")), - queueBatchStateStore: NewQueueBatchStateStore(db, scope.SubScope("queue_batch_state_store")), - buildStore: NewBuildStore(db, scope.SubScope("build_store")), - requestLogStore: NewRequestLogStore(db, scope.SubScope("request_log_store")), - requestSummaryStore: NewRequestSummaryStore(db, scope.SubScope("request_summary_store")), - requestQueueStore: NewRequestQueueSummaryStore(db, scope.SubScope("request_queue_summary_store")), - requestURIStore: NewRequestURIStore(db, scope.SubScope("request_uri_store")), - }, nil } -// GetRequestStore returns the MySQL-backed RequestStore. -func (f *mysqlStorage) GetRequestStore() storage.RequestStore { +// Verify boundStorage implements the queue-scoped aggregate at compile time. +var _ storage.Storage = (*boundStorage)(nil) + +// GetRequestStore returns the bound MySQL-backed RequestStore. +func (f *boundStorage) GetRequestStore() storage.RequestStore { return f.requestStore } -// GetRequestBatchStore returns the MySQL-backed RequestBatchStore. -func (f *mysqlStorage) GetRequestBatchStore() storage.RequestBatchStore { +// GetRequestBatchStore returns the bound MySQL-backed RequestBatchStore. +func (f *boundStorage) GetRequestBatchStore() storage.RequestBatchStore { return f.requestBatchStore } -// GetChangeStore returns the MySQL-backed ChangeStore. -func (f *mysqlStorage) GetChangeStore() storage.ChangeStore { +// GetChangeStore returns the bound MySQL-backed ChangeStore. +func (f *boundStorage) GetChangeStore() storage.ChangeStore { return f.changeStore } -// GetBatchStore returns the MySQL-backed BatchStore. -func (f *mysqlStorage) GetBatchStore() storage.BatchStore { +// GetBatchStore returns the bound MySQL-backed BatchStore. +func (f *boundStorage) GetBatchStore() storage.BatchStore { return f.batchStore } -// GetBatchDependentStore returns the MySQL-backed BatchDependentStore. -func (f *mysqlStorage) GetBatchDependentStore() storage.BatchDependentStore { +// GetBatchDependentStore returns the bound MySQL-backed BatchDependentStore. +func (f *boundStorage) GetBatchDependentStore() storage.BatchDependentStore { return f.batchDependentStore } -// GetQueueBatchStateStore returns the MySQL-backed QueueBatchStateStore. -func (f *mysqlStorage) GetQueueBatchStateStore() storage.QueueBatchStateStore { +// GetQueueBatchStateStore returns the bound MySQL-backed QueueBatchStateStore. +func (f *boundStorage) GetQueueBatchStateStore() storage.QueueBatchStateStore { return f.queueBatchStateStore } -// GetBuildStore returns the MySQL-backed BuildStore. -func (f *mysqlStorage) GetBuildStore() storage.BuildStore { +// GetBuildStore returns the bound MySQL-backed BuildStore. +func (f *boundStorage) GetBuildStore() storage.BuildStore { return f.buildStore } -// GetRequestLogStore returns the MySQL-backed RequestLogStore. -func (f *mysqlStorage) GetRequestLogStore() storage.RequestLogStore { - return f.requestLogStore -} - -// GetRequestSummaryStore returns the MySQL-backed RequestSummaryStore. -func (f *mysqlStorage) GetRequestSummaryStore() storage.RequestSummaryStore { - return f.requestSummaryStore -} - -// GetRequestQueueSummaryStore returns the MySQL-backed RequestQueueSummaryStore. -func (f *mysqlStorage) GetRequestQueueSummaryStore() storage.RequestQueueSummaryStore { +// GetRequestQueueSummaryStore returns the bound MySQL-backed RequestQueueSummaryStore. +func (f *boundStorage) GetRequestQueueSummaryStore() storage.RequestQueueSummaryStore { return f.requestQueueStore } - -// GetRequestURIStore returns the MySQL-backed RequestURIStore. -func (f *mysqlStorage) GetRequestURIStore() storage.RequestURIStore { - return f.requestURIStore -} - -// Close closes the underlying database connection. -func (f *mysqlStorage) Close() error { - return f.db.Close() -} diff --git a/submitqueue/extension/storage/mysql/storage_test.go b/submitqueue/extension/storage/mysql/storage_test.go index 5dc14a0b..bd29d394 100644 --- a/submitqueue/extension/storage/mysql/storage_test.go +++ b/submitqueue/extension/storage/mysql/storage_test.go @@ -36,16 +36,23 @@ func TestNewStorage(t *testing.T) { s, err := NewStorage(db, testMetrics()) require.NoError(t, err) - assert.NotNil(t, s.GetRequestStore()) - assert.NotNil(t, s.GetRequestBatchStore()) - assert.NotNil(t, s.GetChangeStore()) - assert.NotNil(t, s.GetBatchStore()) - assert.NotNil(t, s.GetBatchDependentStore()) - assert.NotNil(t, s.GetBuildStore()) assert.NotNil(t, s.GetRequestLogStore()) assert.NotNil(t, s.GetRequestSummaryStore()) - assert.NotNil(t, s.GetRequestQueueSummaryStore()) assert.NotNil(t, s.GetRequestURIStore()) + + bound, err := s.For("monorepo") + require.NoError(t, err) + assert.NotNil(t, bound.GetRequestStore()) + assert.NotNil(t, bound.GetRequestBatchStore()) + assert.NotNil(t, bound.GetChangeStore()) + assert.NotNil(t, bound.GetBatchStore()) + assert.NotNil(t, bound.GetBatchDependentStore()) + assert.NotNil(t, bound.GetQueueBatchStateStore()) + assert.NotNil(t, bound.GetBuildStore()) + assert.NotNil(t, bound.GetRequestQueueSummaryStore()) + + _, err = s.For("") + assert.Error(t, err, "resolving an empty queue name must fail") } func TestMysqlStorage_Close(t *testing.T) { diff --git a/submitqueue/extension/storage/queue_batch_state_store.go b/submitqueue/extension/storage/queue_batch_state_store.go index e59573ce..0aa4e053 100644 --- a/submitqueue/extension/storage/queue_batch_state_store.go +++ b/submitqueue/extension/storage/queue_batch_state_store.go @@ -38,15 +38,17 @@ import ( // record followed by Delete of the old one, which keeps at least one record visible // throughout. All writes are idempotent so queue redeliveries can safely repeat them. type QueueBatchStateStore interface { - // List returns every record filed under (queue, state). An empty slice means the - // bucket is empty. Order is unspecified. - List(ctx context.Context, queue string, state entity.BatchState) ([]entity.QueueBatchState, error) + // List returns every record filed under the bound queue's given state bucket. + // An empty slice means the bucket is empty. Order is unspecified. + List(ctx context.Context, state entity.BatchState) ([]entity.QueueBatchState, error) - // Put persists a record. Writing an already-existing (queue, state, batchID) record - // is a no-op success, so the call is idempotent under redeliveries. + // Put persists a record. The record's Queue must match the instance's bound + // queue. Writing an already-existing (queue, state, batchID) record is a no-op + // success, so the call is idempotent under redeliveries. Put(ctx context.Context, record entity.QueueBatchState) error - // Delete removes the record identified by (queue, state, batchID). Deleting an - // absent record is a no-op success, so the call is idempotent under redeliveries. - Delete(ctx context.Context, queue string, state entity.BatchState, batchID string) error + // Delete removes the bound queue's record identified by (state, batchID). + // Deleting an absent record is a no-op success, so the call is idempotent + // under redeliveries. + Delete(ctx context.Context, state entity.BatchState, batchID string) error } diff --git a/submitqueue/extension/storage/request_queue_summary_store.go b/submitqueue/extension/storage/request_queue_summary_store.go index d2a7debc..2023dbd6 100644 --- a/submitqueue/extension/storage/request_queue_summary_store.go +++ b/submitqueue/extension/storage/request_queue_summary_store.go @@ -30,10 +30,9 @@ type RequestQueueSummaryCursor struct { RequestID string } -// RequestQueueSummaryQuery specifies one bounded queue-summary page query. +// RequestQueueSummaryQuery specifies one bounded queue-summary page query +// against the bound queue's partition. type RequestQueueSummaryQuery struct { - // Queue is the exact queue partition to scan. - Queue string // ReceivedAtOrAfterMs is the inclusive lower receipt-time bound. ReceivedAtOrAfterMs int64 // ReceivedBeforeMs is the exclusive upper receipt-time bound. @@ -49,10 +48,11 @@ type RequestQueueSummaryQuery struct { // RequestQueueSummaryStore persists the queue-ordered request projection. type RequestQueueSummaryStore interface { // Create inserts summary and returns ErrAlreadyExists when its full primary key already exists. + // The summary's Queue must match the instance's bound queue. Create(ctx context.Context, summary entity.RequestQueueSummary) error - // Get returns the row identified by its full primary key, or ErrNotFound when absent. - Get(ctx context.Context, queue string, receivedAtMs int64, requestID string) (entity.RequestQueueSummary, error) + // Get returns the bound queue's row identified by (receivedAtMs, requestID), or ErrNotFound when absent. + Get(ctx context.Context, receivedAtMs int64, requestID string) (entity.RequestQueueSummary, error) // Update conditionally replaces all non-key fields when the persisted projection version equals oldVersion. // The store writes newVersion exactly as supplied and returns ErrVersionMismatch when the guard does not match. diff --git a/submitqueue/extension/storage/storage.go b/submitqueue/extension/storage/storage.go index 7b03806e..aaefe189 100644 --- a/submitqueue/extension/storage/storage.go +++ b/submitqueue/extension/storage/storage.go @@ -44,7 +44,34 @@ var ErrAlreadyExists = errors.New("record already exists") // and either retry or implement idempotent operations. It is intrinsically a retryable infrastructure error. var ErrVersionMismatch = errs.NewRetryableError(errors.New("version mismatch")) -// Storage is a factory interface that aggregates all entity stores into a single injectable dependency. +// Config identifies the queue a Storage 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 data the resolved Storage is + // scoped to. + QueueName string +} + +// Factory resolves the queue-scoped Storage aggregate 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 write that queue's data. +type Factory interface { + // For returns the Storage aggregate bound to the queue named in config. + For(config Config) (Storage, error) +} + +// Storage aggregates the queue-scoped entity stores into a single injectable +// 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 @@ -67,18 +94,6 @@ type Storage interface { // GetBuildStore returns the BuildStore instance. GetBuildStore() BuildStore - // GetRequestLogStore returns the RequestLogStore instance. - GetRequestLogStore() RequestLogStore - - // GetRequestSummaryStore returns the RequestSummaryStore instance. - GetRequestSummaryStore() RequestSummaryStore - // GetRequestQueueSummaryStore returns the RequestQueueSummaryStore instance. GetRequestQueueSummaryStore() RequestQueueSummaryStore - - // GetRequestURIStore returns the RequestURIStore instance. - GetRequestURIStore() RequestURIStore - - // Close closes the storage and all underlying connections. Should only be called once at the end of the program. - Close() error } diff --git a/submitqueue/gateway/controller/BUILD.bazel b/submitqueue/gateway/controller/BUILD.bazel index 282d8950..5a3f2381 100644 --- a/submitqueue/gateway/controller/BUILD.bazel +++ b/submitqueue/gateway/controller/BUILD.bazel @@ -49,8 +49,10 @@ go_test( "//platform/base/messagequeue:go_default_library", "//platform/consumer:go_default_library", "//platform/errs: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/request:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/queueconfig:go_default_library", diff --git a/submitqueue/gateway/controller/cancel.go b/submitqueue/gateway/controller/cancel.go index 6bfcd60e..ba4589f7 100644 --- a/submitqueue/gateway/controller/cancel.go +++ b/submitqueue/gateway/controller/cancel.go @@ -52,12 +52,12 @@ type cancelController struct { // 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, store storage.Storage, registry consumer.TopicRegistry) CancelController { +func NewCancelController(logger *zap.SugaredLogger, scope tally.Scope, summaries storage.RequestSummaryStore, materializer *requestcore.Materializer, registry consumer.TopicRegistry) CancelController { return &cancelController{ logger: logger, metricsScope: scope, - requestSummaryStore: store.GetRequestSummaryStore(), - materializer: requestcore.NewMaterializer(store), + requestSummaryStore: summaries, + materializer: materializer, registry: registry, } } diff --git a/submitqueue/gateway/controller/cancel_test.go b/submitqueue/gateway/controller/cancel_test.go index e0c76751..1e2328ba 100644 --- a/submitqueue/gateway/controller/cancel_test.go +++ b/submitqueue/gateway/controller/cancel_test.go @@ -35,6 +35,12 @@ import ( // newCancelTestRegistry builds a single-entry TopicRegistry for TopicKeyCancel wired // to a mock Queue/Publisher and returns both the registry and the publisher mock. +// 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) +} + func newCancelTestRegistry(t *testing.T, ctrl *gomock.Controller) (consumer.TopicRegistry, *queuemock.MockPublisher) { t.Helper() pub := queuemock.NewMockPublisher(ctrl) @@ -76,7 +82,7 @@ func testCancelRequest(sqid string, reason string) entity.CancelRequest { func TestNewCancelController(t *testing.T) { ctrl := gomock.NewController(t) - controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newCancelStorageFixture(ctrl, "test-queue/42").storage, newCancelTestRegistryWithNoopPublisher(t, ctrl)) + controller := newTestCancelController(ctrl, tally.NoopScope, newCancelStorageFixture(ctrl, "test-queue/42"), newCancelTestRegistryWithNoopPublisher(t, ctrl)) require.NotNil(t, controller) } @@ -84,7 +90,7 @@ func TestCancel_HappyPath(t *testing.T) { ctrl := gomock.NewController(t) scope := tally.NewTestScope("gateway", nil) - controller := NewCancelController(zap.NewNop().Sugar(), scope, newCancelStorageFixture(ctrl, "test-queue/42").storage, newCancelTestRegistryWithNoopPublisher(t, ctrl)) + 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")) @@ -115,7 +121,7 @@ func TestCancel_HappyPath(t *testing.T) { func TestCancel_ReturnsErrorOnEmptySqid(t *testing.T) { ctrl := gomock.NewController(t) - controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newCancelStorageFixture(ctrl, "test-queue/42").storage, newCancelTestRegistryWithNoopPublisher(t, ctrl)) + controller := newTestCancelController(ctrl, tally.NoopScope, newCancelStorageFixture(ctrl, "test-queue/42"), newCancelTestRegistryWithNoopPublisher(t, ctrl)) ctx := context.Background() err := controller.Cancel(ctx, testCancelRequest("", "anything")) @@ -139,7 +145,7 @@ func TestCancel_PublishesToQueue(t *testing.T) { }, ) - controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newCancelStorageFixture(ctrl, "my-queue/7").storage, registry) + controller := newTestCancelController(ctrl, tally.NoopScope, newCancelStorageFixture(ctrl, "my-queue/7"), registry) ctx := context.Background() err := controller.Cancel(ctx, testCancelRequest("my-queue/7", "obsolete change")) @@ -173,7 +179,7 @@ func TestCancel_InsertsCancellingLog(t *testing.T) { }, ) - controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, fixture.storage, registry) + controller := newTestCancelController(ctrl, tally.NoopScope, fixture, registry) err := controller.Cancel(context.Background(), testCancelRequest("my-queue/42", "obsolete change")) require.NoError(t, err) @@ -198,7 +204,7 @@ func TestCancel_LogInsertFailure(t *testing.T) { registry, publisher := newCancelTestRegistry(t, ctrl) _ = publisher - controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, fixture.storage, registry) + controller := newTestCancelController(ctrl, tally.NoopScope, fixture, registry) err := controller.Cancel(context.Background(), testCancelRequest("q/1", "")) require.Error(t, err) } @@ -209,7 +215,7 @@ func TestCancel_ReturnsErrorOnPublishFailure(t *testing.T) { registry, publisher := newCancelTestRegistry(t, ctrl) publisher.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("queue unavailable")) - controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newCancelStorageFixture(ctrl, "test-queue/1").storage, registry) + controller := newTestCancelController(ctrl, tally.NoopScope, newCancelStorageFixture(ctrl, "test-queue/1"), registry) ctx := context.Background() err := controller.Cancel(ctx, testCancelRequest("test-queue/1", "")) @@ -224,7 +230,7 @@ func TestCancel_UnknownSqidIsUserError(t *testing.T) { registry, publisher := newCancelTestRegistry(t, ctrl) _ = publisher - controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, fixture.storage, registry) + controller := newTestCancelController(ctrl, tally.NoopScope, fixture, registry) err := controller.Cancel(context.Background(), testCancelRequest("ghost/1", "")) require.Error(t, err) assert.True(t, IsRequestNotFound(err)) @@ -242,15 +248,13 @@ func TestCancel_UnknownSqidIsUserError(t *testing.T) { func TestCancel_RequestSummaryLookupFailure(t *testing.T) { ctrl := gomock.NewController(t) - store := storagemock.NewMockStorage(ctrl) summaryStore := storagemock.NewMockRequestSummaryStore(ctrl) - store.EXPECT().GetRequestSummaryStore().Return(summaryStore) summaryStore.EXPECT().Get(gomock.Any(), "q/1").Return(entity.RequestSummary{}, fmt.Errorf("summary backend down")) registry, publisher := newCancelTestRegistry(t, ctrl) _ = publisher - controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, store, registry) + controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, summaryStore, newControllerStorageFixture(ctrl).newMaterializer(ctrl), registry) err := controller.Cancel(context.Background(), testCancelRequest("q/1", "")) require.Error(t, err) assert.False(t, errs.IsUserError(err)) diff --git a/submitqueue/gateway/controller/land.go b/submitqueue/gateway/controller/land.go index 114e1e3a..a736e1de 100644 --- a/submitqueue/gateway/controller/land.go +++ b/submitqueue/gateway/controller/land.go @@ -72,7 +72,7 @@ type landController struct { logger *zap.SugaredLogger metricsScope tally.Scope counter counter.Counter - store storage.Storage + summaries storage.RequestSummaryStore materializer *requestcore.Materializer queueConfigs queueconfig.Store registry consumer.TopicRegistry @@ -81,13 +81,13 @@ 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, store storage.Storage, queueConfigs queueconfig.Store, registry consumer.TopicRegistry) LandController { +func NewLandController(logger *zap.SugaredLogger, scope tally.Scope, counter counter.Counter, summaries storage.RequestSummaryStore, materializer *requestcore.Materializer, queueConfigs queueconfig.Store, registry consumer.TopicRegistry) LandController { return &landController{ logger: logger, metricsScope: scope.SubScope("land_controller"), counter: counter, - store: store, - materializer: requestcore.NewMaterializer(store), + summaries: summaries, + materializer: materializer, queueConfigs: queueConfigs, registry: registry, } @@ -138,7 +138,7 @@ func (c *landController) Land(ctx context.Context, req entity.LandRequest) (resu Version: 1, Metadata: map[string]string{}, } - if err := c.store.GetRequestSummaryStore().Create(ctx, summary); err != nil { + if err := c.summaries.Create(ctx, summary); err != nil { return entity.LandResult{}, fmt.Errorf("failed to create request receipt sqid=%s: %w", req.ID, err) } diff --git a/submitqueue/gateway/controller/land_test.go b/submitqueue/gateway/controller/land_test.go index 80fcc081..53ddd59d 100644 --- a/submitqueue/gateway/controller/land_test.go +++ b/submitqueue/gateway/controller/land_test.go @@ -28,8 +28,10 @@ import ( entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" "github.com/uber/submitqueue/platform/consumer" "github.com/uber/submitqueue/platform/errs" + "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" + requestcore "github.com/uber/submitqueue/submitqueue/core/request" "github.com/uber/submitqueue/submitqueue/core/topickey" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/queueconfig" @@ -66,8 +68,9 @@ func newTestRegistryWithNoopPublisher(t *testing.T, ctrl *gomock.Controller) con } // noopStorage returns stateful request storage whose writes succeed. -func noopStorage(ctrl *gomock.Controller) storage.Storage { - return newControllerStorageFixture(ctrl).storage +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)) } // noopQueueConfigStore returns a mock queueconfig.Store that always reports @@ -92,7 +95,7 @@ func TestNewLandController(t *testing.T) { ctrl := gomock.NewController(t) cnt := countermock.NewMockCounter(ctrl) - controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), noopQueueConfigStore(ctrl), newTestRegistryWithNoopPublisher(t, ctrl)) + controller := newNoopLandController(t, ctrl, cnt) require.NotNil(t, controller) } @@ -101,7 +104,7 @@ func TestLand_ReturnsSqid(t *testing.T) { cnt := countermock.NewMockCounter(ctrl) cnt.EXPECT().Next(gomock.Any(), gomock.Any()).Return(int64(1), nil) - controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), noopQueueConfigStore(ctrl), newTestRegistryWithNoopPublisher(t, ctrl)) + controller := newNoopLandController(t, ctrl, cnt) ctx := context.Background() result, err := controller.Land(ctx, testLandRequest("test-queue")) @@ -115,7 +118,7 @@ func TestLand_ReturnsErrorOnCounterFailure(t *testing.T) { cnt := countermock.NewMockCounter(ctrl) cnt.EXPECT().Next(gomock.Any(), gomock.Any()).Return(int64(0), fmt.Errorf("counter unavailable")) - controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), noopQueueConfigStore(ctrl), newTestRegistryWithNoopPublisher(t, ctrl)) + controller := newNoopLandController(t, ctrl, cnt) ctx := context.Background() _, err := controller.Land(ctx, testLandRequest("test-queue")) @@ -135,7 +138,7 @@ func TestLand_CounterDomainIncludesQueue(t *testing.T) { return 1, nil }, ) - controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), noopQueueConfigStore(ctrl), newTestRegistryWithNoopPublisher(t, ctrl)) + controller := newNoopLandController(t, ctrl, cnt) ctx := context.Background() _, err := controller.Land(ctx, testLandRequest("my-queue")) @@ -148,7 +151,7 @@ func TestLand_ReturnsErrorOnEmptyQueue(t *testing.T) { ctrl := gomock.NewController(t) cnt := countermock.NewMockCounter(ctrl) - controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), noopQueueConfigStore(ctrl), newTestRegistryWithNoopPublisher(t, ctrl)) + controller := newNoopLandController(t, ctrl, cnt) ctx := context.Background() req := testLandRequest("") @@ -175,14 +178,7 @@ func TestLand_ValidatesQueueLengthBeforeAllocatingSqid(t *testing.T) { if !tt.wantError { cnt.EXPECT().Next(gomock.Any(), gomock.Any()).Return(int64(1), nil) } - controller := NewLandController( - zap.NewNop().Sugar(), - tally.NoopScope, - cnt, - noopStorage(ctrl), - noopQueueConfigStore(ctrl), - newTestRegistryWithNoopPublisher(t, ctrl), - ) + controller := newNoopLandController(t, ctrl, cnt) result, err := controller.Land(context.Background(), entity.LandRequest{ Queue: tt.queue, @@ -204,7 +200,7 @@ func TestLand_ReturnsErrorOnEmptyChangeUri(t *testing.T) { ctrl := gomock.NewController(t) cnt := countermock.NewMockCounter(ctrl) - controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), noopQueueConfigStore(ctrl), newTestRegistryWithNoopPublisher(t, ctrl)) + controller := newNoopLandController(t, ctrl, cnt) ctx := context.Background() req := entity.LandRequest{ @@ -230,14 +226,7 @@ func TestLand_ReturnsErrorOnInvalidChangeURIs(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctrl := gomock.NewController(t) - controller := NewLandController( - zap.NewNop().Sugar(), - tally.NoopScope, - countermock.NewMockCounter(ctrl), - noopStorage(ctrl), - noopQueueConfigStore(ctrl), - newTestRegistryWithNoopPublisher(t, ctrl), - ) + controller := newNoopLandController(t, ctrl, countermock.NewMockCounter(ctrl)) _, err := controller.Land(context.Background(), entity.LandRequest{ Queue: "test-queue", @@ -254,7 +243,7 @@ func TestLand_ReturnsErrorOnZeroValueChange(t *testing.T) { ctrl := gomock.NewController(t) cnt := countermock.NewMockCounter(ctrl) - controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), noopQueueConfigStore(ctrl), newTestRegistryWithNoopPublisher(t, ctrl)) + controller := newNoopLandController(t, ctrl, cnt) ctx := context.Background() req := entity.LandRequest{ @@ -274,7 +263,8 @@ func TestLand_ReturnsUnrecognizedQueueWhenStoreReportsNotFound(t *testing.T) { qcs := qcmock.NewMockStore(ctrl) qcs.EXPECT().Get(gomock.Any(), "missing-queue").Return(entity.QueueConfig{}, queueconfig.ErrNotFound) - controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), qcs, newTestRegistryWithNoopPublisher(t, ctrl)) + fixture := newControllerStorageFixture(ctrl) + controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, fixture.summaryStore, fixture.newMaterializer(ctrl), qcs, newTestRegistryWithNoopPublisher(t, ctrl)) ctx := context.Background() _, err := controller.Land(ctx, testLandRequest("missing-queue")) @@ -296,7 +286,8 @@ func TestLand_PropagatesQueueConfigStoreError(t *testing.T) { qcs := qcmock.NewMockStore(ctrl) qcs.EXPECT().Get(gomock.Any(), "test-queue").Return(entity.QueueConfig{}, fmt.Errorf("config backend down")) - controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), qcs, newTestRegistryWithNoopPublisher(t, ctrl)) + fixture := newControllerStorageFixture(ctrl) + controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, fixture.summaryStore, fixture.newMaterializer(ctrl), qcs, newTestRegistryWithNoopPublisher(t, ctrl)) ctx := context.Background() _, err := controller.Land(ctx, testLandRequest("test-queue")) @@ -325,10 +316,10 @@ func TestLand_PublishesToQueue(t *testing.T) { uriStore := storagemock.NewMockRequestURIStore(ctrl) queueStore := storagemock.NewMockRequestQueueSummaryStore(ctrl) logStore := storagemock.NewMockRequestLogStore(ctrl) - store.EXPECT().GetRequestSummaryStore().Return(summaryStore).AnyTimes() - store.EXPECT().GetRequestURIStore().Return(uriStore).AnyTimes() store.EXPECT().GetRequestQueueSummaryStore().Return(queueStore).AnyTimes() - store.EXPECT().GetRequestLogStore().Return(logStore).AnyTimes() + factory := storagemock.NewMockFactory(ctrl) + factory.EXPECT().For(gomock.Any()).Return(store, nil).AnyTimes() + materializer := requestcore.NewMaterializer(logStore, summaryStore, uriStore, factory) registry, publisher := newTestRegistry(t, ctrl) gomock.InOrder( @@ -363,8 +354,8 @@ func TestLand_PublishesToQueue(t *testing.T) { return nil }, ), - queueStore.EXPECT().Get(gomock.Any(), "test-queue", gomock.Any(), "test-queue/123").DoAndReturn( - func(context.Context, string, int64, string) (entity.RequestQueueSummary, error) { + queueStore.EXPECT().Get(gomock.Any(), gomock.Any(), "test-queue/123").DoAndReturn( + func(context.Context, int64, string) (entity.RequestQueueSummary, error) { return entity.RequestQueueSummary{}, storage.ErrNotFound }, ), @@ -382,7 +373,7 @@ func TestLand_PublishesToQueue(t *testing.T) { ), ) - controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, store, noopQueueConfigStore(ctrl), registry) + controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, summaryStore, materializer, noopQueueConfigStore(ctrl), registry) ctx := context.Background() req := entity.LandRequest{ @@ -449,9 +440,7 @@ func TestLand_ReturnsErrorWhenPublishFails(t *testing.T) { cnt := countermock.NewMockCounter(ctrl) cnt.EXPECT().Next(gomock.Any(), gomock.Any()).Return(int64(999), nil) - store := storagemock.NewMockStorage(ctrl) summaryStore := storagemock.NewMockRequestSummaryStore(ctrl) - store.EXPECT().GetRequestSummaryStore().Return(summaryStore) summaryStore.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, summary entity.RequestSummary) error { assert.Equal(t, entity.RequestStatusAccepting, summary.Status) return nil @@ -460,7 +449,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, store, noopQueueConfigStore(ctrl), registry) + controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, summaryStore, newControllerStorageFixture(ctrl).newMaterializer(ctrl), noopQueueConfigStore(ctrl), registry) ctx := context.Background() _, err := controller.Land(ctx, testLandRequest("test-queue")) @@ -479,7 +468,8 @@ func TestLand_ReturnsSqidWhenAcceptedLogFailsAfterPublish(t *testing.T) { zap.NewNop().Sugar(), tally.NoopScope, cnt, - fixture.storage, + fixture.summaryStore, + fixture.newMaterializer(ctrl), noopQueueConfigStore(ctrl), newTestRegistryWithNoopPublisher(t, ctrl), ) diff --git a/submitqueue/gateway/controller/list.go b/submitqueue/gateway/controller/list.go index 657a216e..ebad32dc 100644 --- a/submitqueue/gateway/controller/list.go +++ b/submitqueue/gateway/controller/list.go @@ -52,19 +52,19 @@ type ListController interface { var _ ListController = (*listController)(nil) type listController struct { - logger *zap.SugaredLogger - metricsScope tally.Scope - requestQueueSummaryStore storage.RequestQueueSummaryStore - queueConfigs queueconfig.Store + logger *zap.SugaredLogger + metricsScope tally.Scope + stores storage.Factory + queueConfigs queueconfig.Store } // NewListController creates a gateway list controller. -func NewListController(logger *zap.SugaredLogger, scope tally.Scope, requestQueueSummaryStore storage.RequestQueueSummaryStore, queueConfigs queueconfig.Store) ListController { +func NewListController(logger *zap.SugaredLogger, scope tally.Scope, stores storage.Factory, queueConfigs queueconfig.Store) ListController { return &listController{ - logger: logger, - metricsScope: scope.SubScope("list_controller"), - requestQueueSummaryStore: requestQueueSummaryStore, - queueConfigs: queueConfigs, + logger: logger, + metricsScope: scope.SubScope("list_controller"), + stores: stores, + queueConfigs: queueConfigs, } } @@ -93,8 +93,12 @@ func (c *listController) List(ctx context.Context, req entity.ListRequest) (resu return entity.ListResult{}, fmt.Errorf("page_size must be between 0 and %d: %w", maxListPageSize, ErrInvalidRequest) } + store, err := c.stores.For(storage.Config{QueueName: req.Queue}) + if err != nil { + return entity.ListResult{}, fmt.Errorf("failed to resolve storage for queue %q: %w", req.Queue, err) + } + query := storage.RequestQueueSummaryQuery{ - Queue: req.Queue, ReceivedAtOrAfterMs: req.ReceivedAtOrAfterMs, ReceivedBeforeMs: req.ReceivedBeforeMs, Limit: pageSize + 1, @@ -111,7 +115,7 @@ func (c *listController) List(ctx context.Context, req entity.ListRequest) (resu query.Cursor = storage.RequestQueueSummaryCursor{ReceivedAtMs: token.LastReceivedAtMs, RequestID: token.LastRequestID} } - summaries, err := c.requestQueueSummaryStore.List(ctx, query) + summaries, err := store.GetRequestQueueSummaryStore().List(ctx, query) if err != nil { return entity.ListResult{}, fmt.Errorf("failed to list queue=%s: %w", req.Queue, err) } diff --git a/submitqueue/gateway/controller/list_test.go b/submitqueue/gateway/controller/list_test.go index 2a9741b3..9f83042d 100644 --- a/submitqueue/gateway/controller/list_test.go +++ b/submitqueue/gateway/controller/list_test.go @@ -32,11 +32,21 @@ import ( "go.uber.org/zap" ) +// listFactoryFor wraps a queue-summary store in a storage.Factory that +// resolves every queue to an aggregate exposing it. +func listFactoryFor(ctrl *gomock.Controller, store storage.RequestQueueSummaryStore) storage.Factory { + agg := storagemock.NewMockStorage(ctrl) + agg.EXPECT().GetRequestQueueSummaryStore().Return(store).AnyTimes() + f := storagemock.NewMockFactory(ctrl) + f.EXPECT().For(gomock.Any()).Return(agg, nil).AnyTimes() + return f +} + func TestList_ReturnsPageAndCursor(t *testing.T) { ctrl := gomock.NewController(t) store := storagemock.NewMockRequestQueueSummaryStore(ctrl) store.EXPECT().List(gomock.Any(), storage.RequestQueueSummaryQuery{ - Queue: "q", ReceivedAtOrAfterMs: 100, ReceivedBeforeMs: 200, Limit: 3, + ReceivedAtOrAfterMs: 100, ReceivedBeforeMs: 200, Limit: 3, }).Return([]entity.RequestQueueSummary{ {RequestID: "q/3", Queue: "q", ChangeURIs: []string{}, ReceivedAtMs: 190, Status: entity.RequestStatusAccepted, Metadata: map[string]string{}}, {RequestID: "q/2", Queue: "q", ChangeURIs: []string{}, ReceivedAtMs: 180, Status: entity.RequestStatusLanded, Metadata: map[string]string{}}, @@ -62,7 +72,7 @@ func TestList_UsesCursor(t *testing.T) { store := storagemock.NewMockRequestQueueSummaryStore(ctrl) token := encodeListPageToken(listPageToken{Queue: "q", ReceivedAtOrAfterMs: 100, ReceivedBeforeMs: 200, LastReceivedAtMs: 180, LastRequestID: "q/2"}) store.EXPECT().List(gomock.Any(), storage.RequestQueueSummaryQuery{ - Queue: "q", ReceivedAtOrAfterMs: 100, ReceivedBeforeMs: 200, Limit: 51, + ReceivedAtOrAfterMs: 100, ReceivedBeforeMs: 200, Limit: 51, HasCursor: true, Cursor: storage.RequestQueueSummaryCursor{ReceivedAtMs: 180, RequestID: "q/2"}, }).Return([]entity.RequestQueueSummary{}, nil) controller := newConfiguredListController(ctrl, store) @@ -99,7 +109,7 @@ func TestList_Errors(t *testing.T) { name: "store failure", request: entity.ListRequest{Queue: "q", ReceivedAtOrAfterMs: 1, ReceivedBeforeMs: 2}, setup: func(store *storagemock.MockRequestQueueSummaryStore) { - store.EXPECT().List(gomock.Any(), storage.RequestQueueSummaryQuery{Queue: "q", ReceivedAtOrAfterMs: 1, ReceivedBeforeMs: 2, Limit: 51}).Return(nil, backendErr) + store.EXPECT().List(gomock.Any(), storage.RequestQueueSummaryQuery{ReceivedAtOrAfterMs: 1, ReceivedBeforeMs: 2, Limit: 51}).Return(nil, backendErr) }, }, } @@ -119,7 +129,7 @@ func TestList_Errors(t *testing.T) { if tt.setup != nil { tt.setup(store) } - controller := NewListController(zap.NewNop().Sugar(), tally.NoopScope, store, queueConfigs) + controller := NewListController(zap.NewNop().Sugar(), tally.NoopScope, listFactoryFor(ctrl, store), queueConfigs) _, err := controller.List(context.Background(), tt.request) require.Error(t, err) if tt.wantInvalid { @@ -133,5 +143,5 @@ func TestList_Errors(t *testing.T) { func newConfiguredListController(ctrl *gomock.Controller, store storage.RequestQueueSummaryStore) ListController { queueConfigs := qcmock.NewMockStore(ctrl) queueConfigs.EXPECT().Get(gomock.Any(), "q").Return(entity.QueueConfig{}, nil) - return NewListController(zap.NewNop().Sugar(), tally.NoopScope, store, queueConfigs) + return NewListController(zap.NewNop().Sugar(), tally.NoopScope, listFactoryFor(ctrl, store), queueConfigs) } diff --git a/submitqueue/gateway/controller/log/BUILD.bazel b/submitqueue/gateway/controller/log/BUILD.bazel index eb1743a8..829cfb33 100644 --- a/submitqueue/gateway/controller/log/BUILD.bazel +++ b/submitqueue/gateway/controller/log/BUILD.bazel @@ -10,7 +10,6 @@ go_library( "//platform/metrics:go_default_library", "//submitqueue/core/request:go_default_library", "//submitqueue/entity:go_default_library", - "//submitqueue/extension/storage:go_default_library", "@com_github_uber_go_tally//:go_default_library", "@org_uber_go_zap//:go_default_library", ], @@ -23,6 +22,7 @@ go_test( deps = [ "//platform/base/messagequeue:go_default_library", "//platform/consumer/mock:go_default_library", + "//submitqueue/core/request:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/storage/mock:go_default_library", diff --git a/submitqueue/gateway/controller/log/log.go b/submitqueue/gateway/controller/log/log.go index a37ba75f..15a63b19 100644 --- a/submitqueue/gateway/controller/log/log.go +++ b/submitqueue/gateway/controller/log/log.go @@ -23,7 +23,6 @@ import ( "github.com/uber/submitqueue/platform/metrics" requestcore "github.com/uber/submitqueue/submitqueue/core/request" "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/storage" "go.uber.org/zap" ) @@ -49,14 +48,14 @@ var _ consumer.Controller = (*Controller)(nil) func NewController( logger *zap.SugaredLogger, scope tally.Scope, - store storage.Storage, + materializer *requestcore.Materializer, topicKey consumer.TopicKey, consumerGroup string, ) *Controller { return &Controller{ logger: logger.Named("log_controller"), metricsScope: scope.SubScope("log_controller"), - materializer: requestcore.NewMaterializer(store), + materializer: materializer, topicKey: topicKey, consumerGroup: consumerGroup, } diff --git a/submitqueue/gateway/controller/log/log_test.go b/submitqueue/gateway/controller/log/log_test.go index 2d8a2a37..47632dd4 100644 --- a/submitqueue/gateway/controller/log/log_test.go +++ b/submitqueue/gateway/controller/log/log_test.go @@ -23,6 +23,7 @@ import ( "github.com/uber-go/tally" entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" consumermock "github.com/uber/submitqueue/platform/consumer/mock" + requestcore "github.com/uber/submitqueue/submitqueue/core/request" "github.com/uber/submitqueue/submitqueue/core/topickey" "github.com/uber/submitqueue/submitqueue/entity" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" @@ -30,31 +31,43 @@ import ( "go.uber.org/zap/zaptest" ) +// 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, + ) +} + func TestController_Process(t *testing.T) { tests := []struct { name string logEntry *entity.RequestLog rawPayload []byte - setupStore func(*gomock.Controller) *storagemock.MockStorage + setupStore func(*gomock.Controller) *requestcore.Materializer wantErr bool }{ { name: "success", logEntry: newRequestLog("test-queue/1", entity.RequestStatusStarted, 1, "", nil), - setupStore: func(ctrl *gomock.Controller) *storagemock.MockStorage { + setupStore: func(ctrl *gomock.Controller) *requestcore.Materializer { return newLogControllerStore(ctrl, nil, nil, nil, nil) }, }, { name: "invalid JSON", rawPayload: []byte(`{"invalid": json"}`), - setupStore: func(ctrl *gomock.Controller) *storagemock.MockStorage { return storagemock.NewMockStorage(ctrl) }, + setupStore: func(ctrl *gomock.Controller) *requestcore.Materializer { return newUnusedMaterializer(ctrl) }, wantErr: true, }, { name: "audit insert failure", logEntry: newRequestLog("test-queue/2", entity.RequestStatusError, 3, "merge conflict", nil), - setupStore: func(ctrl *gomock.Controller) *storagemock.MockStorage { + setupStore: func(ctrl *gomock.Controller) *requestcore.Materializer { return newLogControllerStore(ctrl, fmt.Errorf("audit down"), nil, nil, nil) }, wantErr: true, @@ -62,7 +75,7 @@ func TestController_Process(t *testing.T) { { name: "summary read failure", logEntry: newRequestLog("test-queue/2", entity.RequestStatusError, 3, "merge conflict", nil), - setupStore: func(ctrl *gomock.Controller) *storagemock.MockStorage { + setupStore: func(ctrl *gomock.Controller) *requestcore.Materializer { return newLogControllerStore(ctrl, nil, fmt.Errorf("summary down"), nil, nil) }, wantErr: true, @@ -70,7 +83,7 @@ func TestController_Process(t *testing.T) { { name: "summary update failure", logEntry: newRequestLog("test-queue/2", entity.RequestStatusError, 3, "merge conflict", nil), - setupStore: func(ctrl *gomock.Controller) *storagemock.MockStorage { + setupStore: func(ctrl *gomock.Controller) *requestcore.Materializer { return newLogControllerStore(ctrl, nil, nil, fmt.Errorf("summary update down"), nil) }, wantErr: true, @@ -78,7 +91,7 @@ func TestController_Process(t *testing.T) { { name: "queue projection failure", logEntry: newRequestLog("test-queue/2", entity.RequestStatusError, 3, "merge conflict", nil), - setupStore: func(ctrl *gomock.Controller) *storagemock.MockStorage { + setupStore: func(ctrl *gomock.Controller) *requestcore.Materializer { return newLogControllerStore(ctrl, nil, nil, nil, fmt.Errorf("queue update down")) }, wantErr: true, @@ -110,35 +123,37 @@ func TestController_Process(t *testing.T) { } } -func newLogControllerStore(ctrl *gomock.Controller, insertErr, getErr, updateErr, queueErr error) *storagemock.MockStorage { +func newLogControllerStore(ctrl *gomock.Controller, insertErr, getErr, updateErr, queueErr error) *requestcore.Materializer { store := storagemock.NewMockStorage(ctrl) logStore := storagemock.NewMockRequestLogStore(ctrl) summaryStore := storagemock.NewMockRequestSummaryStore(ctrl) queueStore := storagemock.NewMockRequestQueueSummaryStore(ctrl) - store.EXPECT().GetRequestLogStore().Return(logStore).AnyTimes() - store.EXPECT().GetRequestSummaryStore().Return(summaryStore).AnyTimes() + uriStore := storagemock.NewMockRequestURIStore(ctrl) store.EXPECT().GetRequestQueueSummaryStore().Return(queueStore).AnyTimes() + factory := storagemock.NewMockFactory(ctrl) + factory.EXPECT().For(gomock.Any()).Return(store, nil).AnyTimes() + materializer := requestcore.NewMaterializer(logStore, summaryStore, uriStore, factory) logStore.EXPECT().Insert(gomock.Any(), gomock.Any()).Return(insertErr) if insertErr != nil { - return store + return materializer } summaryStore.EXPECT().Get(gomock.Any(), gomock.Any()).Return(entity.RequestSummary{ RequestID: "test-queue/2", Queue: "test-queue", ChangeURIs: []string{}, ReceivedAtMs: 1, Status: entity.RequestStatusAccepted, StatusTimestampMs: 1, Version: 1, Metadata: map[string]string{}, }, getErr) if getErr != nil { - return store + return materializer } summaryStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(updateErr) if updateErr != nil { - return store + return materializer } - queueStore.EXPECT().Get(gomock.Any(), "test-queue", int64(1), "test-queue/2").Return(entity.RequestQueueSummary{ + queueStore.EXPECT().Get(gomock.Any(), int64(1), "test-queue/2").Return(entity.RequestQueueSummary{ RequestID: "test-queue/2", Queue: "test-queue", ChangeURIs: []string{}, ReceivedAtMs: 1, Status: entity.RequestStatusAccepted, Version: 1, Metadata: map[string]string{}, }, nil) queueStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(1), int32(2)).Return(queueErr) - return store + return materializer } func newRequestLog(requestID string, status entity.RequestStatus, requestVersion int32, lastError string, metadata map[string]string) *entity.RequestLog { diff --git a/submitqueue/gateway/controller/storage_fixture_test.go b/submitqueue/gateway/controller/storage_fixture_test.go index 832aecdc..a1f9dcb2 100644 --- a/submitqueue/gateway/controller/storage_fixture_test.go +++ b/submitqueue/gateway/controller/storage_fixture_test.go @@ -19,6 +19,7 @@ import ( "fmt" "sync" + requestcore "github.com/uber/submitqueue/submitqueue/core/request" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" @@ -49,10 +50,7 @@ func newControllerStorageFixture(ctrl *gomock.Controller) *controllerStorageFixt summaries: make(map[string]entity.RequestSummary), queueSummaries: make(map[string]entity.RequestQueueSummary), } - fixture.storage.EXPECT().GetRequestSummaryStore().Return(fixture.summaryStore).AnyTimes() fixture.storage.EXPECT().GetRequestQueueSummaryStore().Return(fixture.queueStore).AnyTimes() - fixture.storage.EXPECT().GetRequestURIStore().Return(fixture.uriStore).AnyTimes() - fixture.storage.EXPECT().GetRequestLogStore().Return(fixture.logStore).AnyTimes() fixture.summaryStore.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, summary entity.RequestSummary) error { fixture.mu.Lock() @@ -97,14 +95,15 @@ func newControllerStorageFixture(ctrl *gomock.Controller) *controllerStorageFixt fixture.queueSummaries[key] = summary return nil }).AnyTimes() - fixture.queueStore.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, queue string, receivedAtMs int64, requestID string) (entity.RequestQueueSummary, error) { + fixture.queueStore.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, receivedAtMs int64, requestID string) (entity.RequestQueueSummary, error) { fixture.mu.Lock() defer fixture.mu.Unlock() - summary, ok := fixture.queueSummaries[queueSummaryTestKey(queue, receivedAtMs, requestID)] - if !ok { - return entity.RequestQueueSummary{}, storage.ErrNotFound + for _, summary := range fixture.queueSummaries { + if summary.ReceivedAtMs == receivedAtMs && summary.RequestID == requestID { + return summary, nil + } } - return summary, nil + return entity.RequestQueueSummary{}, storage.ErrNotFound }).AnyTimes() fixture.queueStore.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, summary entity.RequestQueueSummary, oldVersion, newVersion int32) error { fixture.mu.Lock() @@ -154,3 +153,17 @@ func (f *controllerStorageFixture) addSummary(summary entity.RequestSummary) { func queueSummaryTestKey(queue string, receivedAtMs int64, requestID string) string { return fmt.Sprintf("%s\x00%d\x00%s", queue, receivedAtMs, requestID) } + +// 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 { + factory := storagemock.NewMockFactory(ctrl) + factory.EXPECT().For(gomock.Any()).Return(f.storage, nil).AnyTimes() + return factory +} + +// newMaterializer builds a request read-model materializer over the fixture's +// global stores and its queue-scoped aggregate. +func (f *controllerStorageFixture) newMaterializer(ctrl *gomock.Controller) *requestcore.Materializer { + return requestcore.NewMaterializer(f.logStore, f.summaryStore, f.uriStore, f.newFactory(ctrl)) +} diff --git a/submitqueue/orchestrator/controller/batch/batch.go b/submitqueue/orchestrator/controller/batch/batch.go index f038a29c..0985063f 100644 --- a/submitqueue/orchestrator/controller/batch/batch.go +++ b/submitqueue/orchestrator/controller/batch/batch.go @@ -41,7 +41,7 @@ type Controller struct { metricsScope tally.Scope registry consumer.TopicRegistry counter counter.Counter - store storage.Storage + stores storage.Factory analyzers conflict.Factory topicKey consumer.TopicKey consumerGroup string @@ -58,7 +58,7 @@ func NewController( scope tally.Scope, registry consumer.TopicRegistry, counter counter.Counter, - store storage.Storage, + stores storage.Factory, analyzers conflict.Factory, topicKey consumer.TopicKey, consumerGroup string, @@ -68,7 +68,7 @@ func NewController( metricsScope: scope.SubScope("batch_controller"), registry: registry, counter: counter, - store: store, + stores: stores, analyzers: analyzers, topicKey: topicKey, consumerGroup: consumerGroup, @@ -88,8 +88,15 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er return fmt.Errorf("failed to deserialize request ID: %w", err) } + store, err := c.stores.For(storage.Config{QueueName: rid.Queue}) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "storage_resolve_errors", 1) + // Non-retryable: a missing or unresolvable queue is a malformed message. + return fmt.Errorf("failed to resolve storage for queue %q: %w", rid.Queue, err) + } + // Fetch request from storage - request, err := c.store.GetRequestStore().Get(ctx, rid.ID) + request, err := store.GetRequestStore().Get(ctx, rid.ID) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to get request %s: %w", rid.ID, err) @@ -146,7 +153,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // the speculation graph downstream. The read goes through the queue's // per-state membership records; classification uses each batch's own // hydrated state, so a stale record can never misreport a batch. - activeBatches, err := corebatch.ListByStates(ctx, c.store, request.Queue, entity.DependencyBatchStates()) + activeBatches, err := corebatch.ListByStates(ctx, store, entity.DependencyBatchStates()) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "batch_store_errors", 1) return fmt.Errorf("failed to get active batches for queue=%s: %w", request.Queue, err) @@ -232,7 +239,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // reconciled by conclude as if it had no requests to act on. newRequestVersion := request.Version + 1 request.State = entity.RequestStateBatched - if err := c.store.GetRequestStore().Update(ctx, request, request.Version, newRequestVersion); err != nil { + if err := store.GetRequestStore().Update(ctx, request, request.Version, newRequestVersion); err != nil { // ErrVersionMismatch == cancel (or another writer) advanced R first. Ack // the message: there is nothing for us to do, and retrying would not help // since the new state of R is now visible to the cancel pipeline. @@ -251,14 +258,14 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er request.Version = newRequestVersion // Persist the batch before creating references to it. A Creating batch is not eligible for dependency analysis or normal processing. - if err := c.store.GetBatchStore().Create(ctx, batch); err != nil { + if err := store.GetBatchStore().Create(ctx, batch); err != nil { metrics.NamedCounter(c.metricsScope, opName, "batch_store_errors", 1) return fmt.Errorf("failed to create batch in batch store: %w", err) } // File the queue's membership record for the new batch so it is // discoverable by state from its first moment in the queue. - if err := corebatch.EnsureRecord(ctx, c.store, batch); err != nil { + if err := corebatch.EnsureRecord(ctx, store, batch); err != nil { metrics.NamedCounter(c.metricsScope, opName, "queue_batch_state_errors", 1) return err } @@ -269,14 +276,14 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er BatchID: batch.ID, Version: 1, } - if err := c.store.GetRequestBatchStore().Create(ctx, association); err != nil { + if err := store.GetRequestBatchStore().Create(ctx, association); err != nil { metrics.NamedCounter(c.metricsScope, opName, "request_batch_store_errors", 1) metrics.NamedCounter(c.metricsScope, opName, "batch_abandoned_creating", 1) return fmt.Errorf("failed to associate request %s with batch %s: %w", requestID, batch.ID, err) } } - batch, err = c.populateBatch(ctx, batch) + batch, err = c.populateBatch(ctx, store, batch) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "batch_abandoned_creating", 1) // Retries intentionally mint a new batch ID. Failures may therefore leave unpublished Creating or Created attempts behind. @@ -324,19 +331,19 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er } // populateBatch creates the reverse-index structure and marks a Creating batch ready for publication. -func (c *Controller) populateBatch(ctx context.Context, batch entity.Batch) (entity.Batch, error) { +func (c *Controller) populateBatch(ctx context.Context, store storage.Storage, batch entity.Batch) (entity.Batch, error) { batchDependent := entity.BatchDependent{ BatchID: batch.ID, Dependents: []string{}, Version: 1, } - if err := c.store.GetBatchDependentStore().Create(ctx, batchDependent); err != nil { + if err := store.GetBatchDependentStore().Create(ctx, batchDependent); err != nil { metrics.NamedCounter(c.metricsScope, opName, "batch_dependent_store_errors", 1) return entity.Batch{}, fmt.Errorf("failed to create batch dependent index for new batchID=%s: %w", batch.ID, err) } for _, dependencyID := range batch.Dependencies { - existing, err := c.store.GetBatchDependentStore().Get(ctx, dependencyID) + existing, err := store.GetBatchDependentStore().Get(ctx, dependencyID) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "batch_dependent_store_errors", 1) return entity.Batch{}, fmt.Errorf("failed to get batch dependent for batchID=%s: %w", dependencyID, err) @@ -346,7 +353,7 @@ func (c *Controller) populateBatch(ctx context.Context, batch entity.Batch) (ent updated.Dependents = append([]string(nil), existing.Dependents...) updated.Dependents = append(updated.Dependents, batch.ID) newVersion := existing.Version + 1 - if err := c.store.GetBatchDependentStore().Update(ctx, updated, existing.Version, newVersion); err != nil { + if err := store.GetBatchDependentStore().Update(ctx, updated, existing.Version, newVersion); err != nil { metrics.NamedCounter(c.metricsScope, opName, "batch_dependent_store_errors", 1) return entity.Batch{}, fmt.Errorf("failed to update batch dependent index for existing batchID=%s and new batchID=%s: %w", dependencyID, batch.ID, err) } @@ -354,7 +361,7 @@ func (c *Controller) populateBatch(ctx context.Context, batch entity.Batch) (ent // The batch's own reverse-index row now exists and every dependency lists this batch as a dependent. // Structural initialization is complete, so transition Creating → Created to make the batch ready for processing once published to speculate. - batch, err := corebatch.Transition(ctx, c.store, batch, entity.BatchStateCreated) + batch, err := corebatch.Transition(ctx, store, batch, entity.BatchStateCreated) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "batch_store_errors", 1) return entity.Batch{}, fmt.Errorf("failed to mark batch %s created: %w", batch.ID, err) diff --git a/submitqueue/orchestrator/controller/batch/batch_test.go b/submitqueue/orchestrator/controller/batch/batch_test.go index 357221ba..33133422 100644 --- a/submitqueue/orchestrator/controller/batch/batch_test.go +++ b/submitqueue/orchestrator/controller/batch/batch_test.go @@ -78,13 +78,13 @@ func newSequentialCounter(ctrl *gomock.Controller) *countermock.MockCounter { func newQueueBatchStateStore(ctrl *gomock.Controller, active ...entity.Batch) *storagemock.MockQueueBatchStateStore { s := storagemock.NewMockQueueBatchStateStore(ctrl) s.EXPECT().Put(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - s.EXPECT().Delete(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - s.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( - func(_ context.Context, queue string, state entity.BatchState) ([]entity.QueueBatchState, error) { + s.EXPECT().Delete(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + s.EXPECT().List(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, state entity.BatchState) ([]entity.QueueBatchState, error) { var records []entity.QueueBatchState for _, b := range active { - if b.Queue == queue && b.State == state { - records = append(records, entity.QueueBatchState{Queue: queue, State: state, BatchID: b.ID}) + if b.State == state { + records = append(records, entity.QueueBatchState{Queue: b.Queue, State: state, BatchID: b.ID}) } } return records, nil @@ -93,6 +93,14 @@ func newQueueBatchStateStore(ctrl *gomock.Controller, active ...entity.Batch) *s return s } +// storageFactoryFor returns a storage.Factory mock that resolves any queue to +// the given queue-scoped store aggregate. +func storageFactoryFor(ctrl *gomock.Controller, store storage.Storage) *storagemock.MockFactory { + f := storagemock.NewMockFactory(ctrl) + f.EXPECT().For(gomock.Any()).Return(store, nil).AnyTimes() + return f +} + // testRequest returns a standard test request for batch tests. func testRequest() entity.Request { return entity.Request{ @@ -167,7 +175,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, mockStorage, analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch") + return NewController(logger, scope, registry, cnt, storageFactoryFor(ctrl, mockStorage), analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch") } func TestNewController(t *testing.T) { @@ -273,7 +281,7 @@ func TestController_Process_StampsQueueOnSpeculatePayload(t *testing.T) { analyzerFactory.EXPECT().For(gomock.Any()).Return(all.New(), nil).AnyTimes() controller := NewController( zaptest.NewLogger(t).Sugar(), tally.NoopScope, registry, newSequentialCounter(ctrl), - mockStorage, analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch", + storageFactoryFor(ctrl, mockStorage), analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch", ) msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) @@ -355,7 +363,7 @@ func TestController_Process_PublishesBatchedLog(t *testing.T) { analyzerFactory.EXPECT().For(gomock.Any()).Return(all.New(), nil).AnyTimes() controller := NewController( zaptest.NewLogger(t).Sugar(), tally.NoopScope, registry, newSequentialCounter(ctrl), - mockStorage, analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch", + storageFactoryFor(ctrl, mockStorage), analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch", ) msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) @@ -810,7 +818,7 @@ func TestController_Process_CASLostToCancel(t *testing.T) { analyzerFactory.EXPECT().For(gomock.Any()).Return(all.New(), nil).AnyTimes() controller := NewController( zaptest.NewLogger(t).Sugar(), tally.NoopScope, registry, newSequentialCounter(ctrl), - mockStorage, analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch", + storageFactoryFor(ctrl, mockStorage), analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch", ) msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) @@ -979,7 +987,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, store, analyzerFactory, + zaptest.NewLogger(t).Sugar(), tally.NoopScope, registry, cnt, storageFactoryFor(ctrl, store), analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch", ) @@ -1183,8 +1191,8 @@ func TestController_PopulateBatch_Errors(t *testing.T) { store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() store.EXPECT().GetBatchDependentStore().Return(batchDependentStore).AnyTimes() - controller := &Controller{metricsScope: tally.NoopScope, store: store} - _, err := controller.populateBatch(context.Background(), batch) + controller := &Controller{metricsScope: tally.NoopScope} + _, err := controller.populateBatch(context.Background(), store, batch) assert.ErrorContains(t, err, tt.errMsg) }) } diff --git a/submitqueue/orchestrator/controller/build/build.go b/submitqueue/orchestrator/controller/build/build.go index 0a1a1e36..0d6e217c 100644 --- a/submitqueue/orchestrator/controller/build/build.go +++ b/submitqueue/orchestrator/controller/build/build.go @@ -36,7 +36,7 @@ import ( type Controller struct { logger *zap.SugaredLogger metricsScope tally.Scope - store storage.Storage + stores storage.Factory buildRunners buildrunner.Factory registry consumer.TopicRegistry topicKey consumer.TopicKey @@ -50,7 +50,7 @@ var _ consumer.Controller = (*Controller)(nil) func NewController( logger *zap.SugaredLogger, scope tally.Scope, - store storage.Storage, + stores storage.Factory, buildRunners buildrunner.Factory, registry consumer.TopicRegistry, topicKey consumer.TopicKey, @@ -59,7 +59,7 @@ func NewController( return &Controller{ logger: logger.Named("build_controller"), metricsScope: scope.SubScope("build_controller"), - store: store, + stores: stores, buildRunners: buildRunners, registry: registry, topicKey: topicKey, @@ -82,8 +82,15 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er return fmt.Errorf("failed to deserialize batch ID: %w", err) } + store, err := c.stores.For(storage.Config{QueueName: bid.Queue}) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "storage_resolve_errors", 1) + // Non-retryable: a missing or unresolvable queue is a malformed message. + return fmt.Errorf("failed to resolve storage for queue %q: %w", bid.Queue, err) + } + // Fetch batch from storage - batch, err := c.store.GetBatchStore().Get(ctx, bid.ID) + batch, err := store.GetBatchStore().Get(ctx, bid.ID) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to get batch %s: %w", bid.ID, err) @@ -121,7 +128,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // Load the dependency batches (base) as identity; the build runner resolves // each batch's changes itself. head is this batch. - base, err := c.loadBatches(ctx, batch.Dependencies) + base, err := c.loadBatches(ctx, store, batch.Dependencies) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to load dependency batches for batch %s: %w", batch.ID, err) @@ -150,7 +157,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // Persist the initial Build snapshot so the buildsignal poll loop has a // row to Update against. ErrAlreadyExists is benign — a redelivery // of this message after a previous successful Create. - if err := c.store.GetBuildStore().Create(ctx, build); err != nil && !errors.Is(err, storage.ErrAlreadyExists) { + if err := store.GetBuildStore().Create(ctx, build); err != nil && !errors.Is(err, storage.ErrAlreadyExists) { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to persist build %s: %w", build.ID, err) } @@ -176,13 +183,13 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // loadBatches loads each batch by ID, preserving order. Used to load the base // (dependency batches) identity handed to BuildRunner.Trigger; the build runner // resolves each batch's changes itself. -func (c *Controller) loadBatches(ctx context.Context, batchIDs []string) ([]entity.Batch, error) { +func (c *Controller) loadBatches(ctx context.Context, store storage.Storage, batchIDs []string) ([]entity.Batch, error) { if len(batchIDs) == 0 { return nil, nil } batches := make([]entity.Batch, 0, len(batchIDs)) for _, bID := range batchIDs { - b, err := c.store.GetBatchStore().Get(ctx, bID) + b, err := store.GetBatchStore().Get(ctx, bID) if err != nil { return nil, fmt.Errorf("failed to get batch %s: %w", bID, err) } diff --git a/submitqueue/orchestrator/controller/build/build_test.go b/submitqueue/orchestrator/controller/build/build_test.go index 3790b17a..3e829460 100644 --- a/submitqueue/orchestrator/controller/build/build_test.go +++ b/submitqueue/orchestrator/controller/build/build_test.go @@ -40,6 +40,12 @@ import ( ) // batchIDPayload serializes a BatchID to JSON bytes for test message payloads. +// staticStorageFactory resolves every queue to one fixed store aggregate. +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 } + func batchIDPayload(t *testing.T, id string) []byte { payload, err := entity.BatchID{ID: id}.ToBytes() require.NoError(t, err) @@ -107,7 +113,7 @@ func newTestController(t *testing.T, ctrl *gomock.Controller, store *storagemock ) require.NoError(t, err) - return NewController(logger, scope, store, staticBuildRunnerFactory{r: br}, registry, topickey.TopicKeyBuild, "orchestrator-build") + return NewController(logger, scope, staticStorageFactory{store: store}, staticBuildRunnerFactory{r: br}, registry, topickey.TopicKeyBuild, "orchestrator-build") } func TestNewController(t *testing.T) { @@ -199,7 +205,7 @@ func TestController_Process_TriggersWithBaseAndHead(t *testing.T) { ) require.NoError(t, err) - controller := NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, store, staticBuildRunnerFactory{r: br}, registry, topickey.TopicKeyBuild, "orchestrator-build") + controller := NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, staticStorageFactory{store: store}, staticBuildRunnerFactory{r: br}, registry, topickey.TopicKeyBuild, "orchestrator-build") msg := entityqueue.NewMessage(headBatch.ID, batchIDPayload(t, headBatch.ID), headBatch.Queue, nil) delivery := consumermock.NewMockDelivery(ctrl) @@ -256,7 +262,7 @@ func TestController_Process_BuildStoreAlreadyExistsIsSwallowed(t *testing.T) { []consumer.TopicConfig{{Key: topickey.TopicKeyBuildSignal, Name: "buildsignal", Queue: mockQ}}, ) require.NoError(t, err) - controller := NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, store, staticBuildRunnerFactory{r: br}, registry, topickey.TopicKeyBuild, "orchestrator-build") + controller := NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, staticStorageFactory{store: store}, staticBuildRunnerFactory{r: br}, registry, topickey.TopicKeyBuild, "orchestrator-build") msg := entityqueue.NewMessage(batch.ID, batchIDPayload(t, batch.ID), batch.Queue, nil) delivery := consumermock.NewMockDelivery(ctrl) @@ -288,7 +294,7 @@ func TestController_Process_TriggerFailure(t *testing.T) { []consumer.TopicConfig{{Key: topickey.TopicKeyBuildSignal, Name: "buildsignal", Queue: queuemock.NewMockQueue(ctrl)}}, ) require.NoError(t, err) - controller := NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, store, staticBuildRunnerFactory{r: br}, registry, topickey.TopicKeyBuild, "orchestrator-build") + controller := NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, staticStorageFactory{store: store}, staticBuildRunnerFactory{r: br}, registry, topickey.TopicKeyBuild, "orchestrator-build") msg := entityqueue.NewMessage(batch.ID, batchIDPayload(t, batch.ID), batch.Queue, nil) delivery := consumermock.NewMockDelivery(ctrl) diff --git a/submitqueue/orchestrator/controller/buildsignal/BUILD.bazel b/submitqueue/orchestrator/controller/buildsignal/BUILD.bazel index 58b23a5b..59534818 100644 --- a/submitqueue/orchestrator/controller/buildsignal/BUILD.bazel +++ b/submitqueue/orchestrator/controller/buildsignal/BUILD.bazel @@ -31,6 +31,7 @@ go_test( "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/buildrunner/mock:go_default_library", + "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mock:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", diff --git a/submitqueue/orchestrator/controller/buildsignal/buildsignal.go b/submitqueue/orchestrator/controller/buildsignal/buildsignal.go index fd3ec821..82604c89 100644 --- a/submitqueue/orchestrator/controller/buildsignal/buildsignal.go +++ b/submitqueue/orchestrator/controller/buildsignal/buildsignal.go @@ -57,7 +57,7 @@ var ( type Controller struct { logger *zap.SugaredLogger metricsScope tally.Scope - store storage.Storage + stores storage.Factory buildRunners buildrunner.Factory registry consumer.TopicRegistry topicKey consumer.TopicKey @@ -71,7 +71,7 @@ var _ consumer.Controller = (*Controller)(nil) func NewController( logger *zap.SugaredLogger, scope tally.Scope, - store storage.Storage, + stores storage.Factory, buildRunners buildrunner.Factory, registry consumer.TopicRegistry, topicKey consumer.TopicKey, @@ -80,7 +80,7 @@ func NewController( return &Controller{ logger: logger.Named("buildsignal_controller"), metricsScope: scope.SubScope("buildsignal_controller"), - store: store, + stores: stores, buildRunners: buildRunners, registry: registry, topicKey: topicKey, @@ -111,10 +111,17 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er return fmt.Errorf("failed to deserialize build ID: %w", err) } + store, err := c.stores.For(storage.Config{QueueName: buildID.Queue}) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "storage_resolve_errors", 1) + // Non-retryable: a missing or unresolvable queue is a malformed message. + return fmt.Errorf("failed to resolve storage for queue %q: %w", buildID.Queue, err) + } + // Only the build ID travels on the queue; load the full Build from // storage, which is the single source of truth for its BatchID and the // snapshot the poll loop updates. - build, err := c.store.GetBuildStore().Get(ctx, buildID.ID) + build, err := store.GetBuildStore().Get(ctx, buildID.ID) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to get build %s: %w", buildID.ID, err) @@ -129,7 +136,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // Load the batch first: it gives us the queue (needed to build the right // BuildRunner) and lets us short-circuit halted batches before polling. - batch, err := c.store.GetBatchStore().Get(ctx, build.BatchID) + batch, err := store.GetBatchStore().Get(ctx, build.BatchID) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to get batch %s: %w", build.BatchID, err) @@ -171,7 +178,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er updatedBuild := build updatedBuild.Status = status - if err := c.store.GetBuildStore().Update(ctx, updatedBuild); err != nil { + if err := store.GetBuildStore().Update(ctx, updatedBuild); err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to update status for build %s: %w", build.ID, err) } diff --git a/submitqueue/orchestrator/controller/buildsignal/buildsignal_test.go b/submitqueue/orchestrator/controller/buildsignal/buildsignal_test.go index 809b2596..2ae812d0 100644 --- a/submitqueue/orchestrator/controller/buildsignal/buildsignal_test.go +++ b/submitqueue/orchestrator/controller/buildsignal/buildsignal_test.go @@ -30,6 +30,7 @@ import ( "github.com/uber/submitqueue/submitqueue/core/topickey" "github.com/uber/submitqueue/submitqueue/entity" buildrunnermock "github.com/uber/submitqueue/submitqueue/extension/buildrunner/mock" + "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" "go.uber.org/mock/gomock" "go.uber.org/zap/zaptest" @@ -47,6 +48,12 @@ type testHarness struct { speculatePub *queuemock.MockPublisher } +// staticStorageFactory resolves every queue to one fixed store aggregate. +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 } + func newTestHarness(t *testing.T, ctrl *gomock.Controller) *testHarness { br := buildrunnermock.NewMockBuildRunner(ctrl) brFactory := buildrunnermock.NewMockFactory(ctrl) @@ -75,7 +82,7 @@ func newTestHarness(t *testing.T, ctrl *gomock.Controller) *testHarness { c := NewController( zaptest.NewLogger(t).Sugar(), tally.NoopScope, - store, + staticStorageFactory{store: store}, brFactory, registry, topickey.TopicKeyBuildSignal, diff --git a/submitqueue/orchestrator/controller/cancel/cancel.go b/submitqueue/orchestrator/controller/cancel/cancel.go index 9629e212..c964aa24 100644 --- a/submitqueue/orchestrator/controller/cancel/cancel.go +++ b/submitqueue/orchestrator/controller/cancel/cancel.go @@ -72,7 +72,7 @@ import ( type Controller struct { logger *zap.SugaredLogger metricsScope tally.Scope - store storage.Storage + stores storage.Factory registry consumer.TopicRegistry topicKey consumer.TopicKey consumerGroup string @@ -87,7 +87,7 @@ const opName = "process" func NewController( logger *zap.SugaredLogger, scope tally.Scope, - store storage.Storage, + stores storage.Factory, registry consumer.TopicRegistry, topicKey consumer.TopicKey, consumerGroup string, @@ -95,7 +95,7 @@ func NewController( return &Controller{ logger: logger.Named("cancel_controller"), metricsScope: scope.SubScope("cancel_controller"), - store: store, + stores: stores, registry: registry, topicKey: topicKey, consumerGroup: consumerGroup, @@ -113,7 +113,14 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er return fmt.Errorf("failed to deserialize cancel request: %w", err) } - request, err := c.store.GetRequestStore().Get(ctx, cancelReq.ID) + store, err := c.stores.For(storage.Config{QueueName: cancelReq.Queue}) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "storage_resolve_errors", 1) + // Non-retryable: a missing or unresolvable queue is a malformed message. + return fmt.Errorf("failed to resolve storage for queue %q: %w", cancelReq.Queue, err) + } + + request, err := store.GetRequestStore().Get(ctx, cancelReq.ID) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to get request %s: %w", cancelReq.ID, err) @@ -145,13 +152,13 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // to RequestStateCancelling. This is non-terminal; forward-progress controllers // (validate, batch) treat it as halted, but conclude may still write a different // terminal state if a concurrent merge or failure wins the race. - request, err = c.markCancelling(ctx, request) + request, err = c.markCancelling(ctx, store, request) if err != nil { return err } // Find every batch associated with this request. Retries may create multiple batch IDs, and each persisted attempt must be handled. - batches, err := c.findBatches(ctx, request) + batches, err := c.findBatches(ctx, store, request) if err != nil { return err } @@ -164,7 +171,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er switch { case batch.State.IsCancellable(): foundApplicableBatch = true - if err := c.cancelBatch(ctx, batch); err != nil { + if err := c.cancelBatch(ctx, store, batch); err != nil { metrics.NamedCounter(c.metricsScope, opName, "batch_cancel_errors", 1) c.logger.Errorw("failed to cancel batch", "batch_id", batch.ID, @@ -187,7 +194,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er } if !foundApplicableBatch { - return c.cancelRequest(ctx, request, cancelReq.Reason) + return c.cancelRequest(ctx, store, request, cancelReq.Reason) } return firstErr } @@ -201,7 +208,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // observing a batch transition) is returned as-is; its declaration makes it // retryable, and the next attempt re-fetches and re-evaluates (it may now // be terminal, in which case the top-level terminal-check acks). -func (c *Controller) markCancelling(ctx context.Context, request entity.Request) (entity.Request, error) { +func (c *Controller) markCancelling(ctx context.Context, store storage.Storage, request entity.Request) (entity.Request, error) { if request.State == entity.RequestStateCancelling { // Idempotent re-delivery: prior pass already recorded intent. metrics.NamedCounter(c.metricsScope, opName, "already_cancelling", 1) @@ -209,7 +216,7 @@ func (c *Controller) markCancelling(ctx context.Context, request entity.Request) } newVersion := request.Version + 1 request.State = entity.RequestStateCancelling - if err := c.store.GetRequestStore().Update(ctx, request, request.Version, newVersion); err != nil { + if err := store.GetRequestStore().Update(ctx, request, request.Version, newVersion); err != nil { metrics.NamedCounter(c.metricsScope, opName, "request_update_errors", 1) return entity.Request{}, fmt.Errorf("failed to mark request %s as cancelling: %w", request.ID, err) } @@ -220,8 +227,8 @@ func (c *Controller) markCancelling(ctx context.Context, request entity.Request) // findBatches resolves every batch attempt associated with the request. // Associations whose batch was never persisted are stale retry artifacts and are ignored. -func (c *Controller) findBatches(ctx context.Context, request entity.Request) ([]entity.Batch, error) { - associations, err := c.store.GetRequestBatchStore().GetByRequestID(ctx, request.ID) +func (c *Controller) findBatches(ctx context.Context, store storage.Storage, request entity.Request) ([]entity.Batch, error) { + associations, err := store.GetRequestBatchStore().GetByRequestID(ctx, request.ID) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "request_batch_store_errors", 1) return nil, fmt.Errorf("failed to get batch associations for request %s: %w", request.ID, err) @@ -229,7 +236,7 @@ func (c *Controller) findBatches(ctx context.Context, request entity.Request) ([ var batches []entity.Batch for _, association := range associations { - batch, err := c.store.GetBatchStore().Get(ctx, association.BatchID) + batch, err := store.GetBatchStore().Get(ctx, association.BatchID) if err != nil { if errors.Is(err, storage.ErrNotFound) { // The association may precede batch persistence or may outlive a failed attempt. @@ -263,12 +270,12 @@ func (c *Controller) findBatches(ctx context.Context, request entity.Request) ([ // concurrent writer already reached a *different* terminal state, the helper // reports TerminationDiverged and we simply ack — the other writer owns the // terminal log for the state it wrote. -func (c *Controller) cancelRequest(ctx context.Context, request entity.Request, reason string) error { +func (c *Controller) cancelRequest(ctx context.Context, store storage.Storage, request entity.Request, reason string) error { metadata := map[string]string{} if reason != "" { metadata["reason"] = reason } - res, err := corerequest.TerminateRequest(ctx, c.store, c.registry, request.ID, entity.RequestStateCancelled, "", metadata) + res, err := corerequest.TerminateRequest(ctx, store, c.registry, request.ID, entity.RequestStateCancelled, "", metadata) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "request_terminate_errors", 1) return fmt.Errorf("failed to cancel request %s: %w", request.ID, err) @@ -311,7 +318,7 @@ func (c *Controller) cancelRequest(ctx context.Context, request entity.Request, // (a prior pass wrote the intent but the publish failed). In that case the // intent CAS is skipped and we just re-publish — speculate absorbs the // duplicate as a cheap no-op nudge. -func (c *Controller) cancelBatch(ctx context.Context, batch entity.Batch) error { +func (c *Controller) cancelBatch(ctx context.Context, store storage.Storage, batch entity.Batch) error { c.logger.Infow("handing batch cancellation off to speculate", "batch_id", batch.ID, "queue", batch.Queue, @@ -320,7 +327,7 @@ func (c *Controller) cancelBatch(ctx context.Context, batch entity.Batch) error if batch.State != entity.BatchStateCancelling { var err error - batch, err = corebatch.Transition(ctx, c.store, batch, entity.BatchStateCancelling) + batch, err = corebatch.Transition(ctx, store, batch, entity.BatchStateCancelling) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "batch_update_errors", 1) // storage.ErrVersionMismatch here means the batch advanced concurrently @@ -335,7 +342,7 @@ func (c *Controller) cancelBatch(ctx context.Context, batch entity.Batch) error metrics.NamedCounter(c.metricsScope, opName, "batch_already_cancelling", 1) // A prior pass wrote the intent but may have crashed before completing // the membership record move; repair before re-publishing. - if err := corebatch.EnsureRecord(ctx, c.store, batch); err != nil { + if err := corebatch.EnsureRecord(ctx, store, batch); err != nil { metrics.NamedCounter(c.metricsScope, opName, "batch_update_errors", 1) return err } diff --git a/submitqueue/orchestrator/controller/cancel/cancel_test.go b/submitqueue/orchestrator/controller/cancel/cancel_test.go index 7d9ac361..07c8fbf6 100644 --- a/submitqueue/orchestrator/controller/cancel/cancel_test.go +++ b/submitqueue/orchestrator/controller/cancel/cancel_test.go @@ -36,10 +36,16 @@ import ( // newQueueBatchStateStore returns a QueueBatchStateStore mock that accepts any // membership-record write; cancel never lists record buckets. +// staticStorageFactory resolves every queue to one fixed store aggregate. +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 } + func newQueueBatchStateStore(ctrl *gomock.Controller) *storagemock.MockQueueBatchStateStore { s := storagemock.NewMockQueueBatchStateStore(ctrl) s.EXPECT().Put(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - s.EXPECT().Delete(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + s.EXPECT().Delete(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() return s } @@ -80,7 +86,7 @@ func newRegistry(t *testing.T, ctrl *gomock.Controller) (consumer.TopicRegistry, } func newController(t *testing.T, store storage.Storage, registry consumer.TopicRegistry) *Controller { - return NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, store, registry, topickey.TopicKeyCancel, "orchestrator-cancel") + return NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, staticStorageFactory{store: store}, registry, topickey.TopicKeyCancel, "orchestrator-cancel") } func newDelivery(t *testing.T, ctrl *gomock.Controller, payload []byte, partitionKey string) consumer.Delivery { @@ -761,8 +767,8 @@ func TestFindBatches(t *testing.T) { store.EXPECT().GetRequestBatchStore().Return(requestBatchStore) store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() - controller := &Controller{metricsScope: tally.NoopScope, store: store} - got, err := controller.findBatches(context.Background(), request) + controller := &Controller{metricsScope: tally.NoopScope} + got, err := controller.findBatches(context.Background(), store, request) if tt.errMsg != "" { assert.ErrorContains(t, err, tt.errMsg) return diff --git a/submitqueue/orchestrator/controller/conclude/conclude.go b/submitqueue/orchestrator/controller/conclude/conclude.go index 453dce46..88e6bf2c 100644 --- a/submitqueue/orchestrator/controller/conclude/conclude.go +++ b/submitqueue/orchestrator/controller/conclude/conclude.go @@ -33,7 +33,7 @@ import ( type Controller struct { logger *zap.SugaredLogger metricsScope tally.Scope - store storage.Storage + stores storage.Factory registry consumer.TopicRegistry topicKey consumer.TopicKey consumerGroup string @@ -46,7 +46,7 @@ var _ consumer.Controller = (*Controller)(nil) func NewController( logger *zap.SugaredLogger, scope tally.Scope, - store storage.Storage, + stores storage.Factory, registry consumer.TopicRegistry, topicKey consumer.TopicKey, consumerGroup string, @@ -54,7 +54,7 @@ func NewController( return &Controller{ logger: logger.Named("conclude_controller"), metricsScope: scope.SubScope("conclude_controller"), - store: store, + stores: stores, registry: registry, topicKey: topicKey, consumerGroup: consumerGroup, @@ -74,8 +74,15 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er return fmt.Errorf("failed to deserialize batch ID: %w", err) } + store, err := c.stores.For(storage.Config{QueueName: bid.Queue}) + if err != nil { + metrics.NamedCounter(c.metricsScope, "process", "storage_resolve_errors", 1) + // Non-retryable: a missing or unresolvable queue is a malformed message. + return fmt.Errorf("failed to resolve storage for queue %q: %w", bid.Queue, err) + } + // Fetch batch from storage - batch, err := c.store.GetBatchStore().Get(ctx, bid.ID) + batch, err := store.GetBatchStore().Get(ctx, bid.ID) if err != nil { metrics.NamedCounter(c.metricsScope, "process", "storage_errors", 1) return fmt.Errorf("failed to get batch %s: %w", bid.ID, err) @@ -117,7 +124,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // retried (and eventually dead-lettered) rather than silently skipped. We // translate the result into per-outcome logs and metrics. for _, requestID := range batch.Contains { - res, err := corerequest.TerminateRequest(ctx, c.store, c.registry, requestID, requestState, "", map[string]string{ + res, err := corerequest.TerminateRequest(ctx, store, c.registry, requestID, requestState, "", map[string]string{ "batch_id": batch.ID, }) if err != nil { diff --git a/submitqueue/orchestrator/controller/conclude/conclude_test.go b/submitqueue/orchestrator/controller/conclude/conclude_test.go index 9b5f361f..72661566 100644 --- a/submitqueue/orchestrator/controller/conclude/conclude_test.go +++ b/submitqueue/orchestrator/controller/conclude/conclude_test.go @@ -35,6 +35,12 @@ import ( "go.uber.org/zap/zaptest" ) +// staticStorageFactory resolves every queue to one fixed store aggregate. +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 } + func requestWithState(request entity.Request, state entity.RequestState) entity.Request { request.State = state return request @@ -77,7 +83,7 @@ func newTestController(t *testing.T, ctrl *gomock.Controller, mockStorage *stora ) require.NoError(t, err) - return NewController(logger, scope, mockStorage, registry, topickey.TopicKeyConclude, "orchestrator-conclude"), mockPub + return NewController(logger, scope, staticStorageFactory{store: mockStorage}, registry, topickey.TopicKeyConclude, "orchestrator-conclude"), mockPub } func TestNewController(t *testing.T) { diff --git a/submitqueue/orchestrator/controller/dlq/batch.go b/submitqueue/orchestrator/controller/dlq/batch.go index 6d5b8e1a..7fe0b5db 100644 --- a/submitqueue/orchestrator/controller/dlq/batch.go +++ b/submitqueue/orchestrator/controller/dlq/batch.go @@ -39,7 +39,7 @@ import ( type batchController struct { logger *zap.SugaredLogger metricsScope tally.Scope - store storage.Storage + stores storage.Factory registry consumer.TopicRegistry topicKey consumer.TopicKey consumerGroup string @@ -53,7 +53,7 @@ var _ consumer.Controller = (*batchController)(nil) func NewDLQBatchController( logger *zap.SugaredLogger, scope tally.Scope, - store storage.Storage, + stores storage.Factory, registry consumer.TopicRegistry, topicKey consumer.TopicKey, consumerGroup string, @@ -62,7 +62,7 @@ func NewDLQBatchController( return &batchController{ logger: logger.Named(name), metricsScope: scope.SubScope(name), - store: store, + stores: stores, registry: registry, topicKey: topicKey, consumerGroup: consumerGroup, @@ -85,6 +85,13 @@ func (c *batchController) Process(ctx context.Context, delivery consumer.Deliver return fmt.Errorf("dlq payload decoded to empty batch id") } + store, err := c.stores.For(storage.Config{QueueName: bid.Queue}) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "storage_resolve_errors", 1) + // Non-retryable: a missing or unresolvable queue is a malformed message. + return fmt.Errorf("failed to resolve storage for queue %q: %w", bid.Queue, err) + } + dmeta := delivery.Metadata() c.logger.Warnw("dlq message received", "batch_id", bid.ID, @@ -94,7 +101,7 @@ func (c *batchController) Process(ctx context.Context, delivery consumer.Deliver "dlq_last_error", dmeta["dlq.last_error"], ) - if err := failBatch(ctx, c.store, c.registry, c.logger, bid.ID, dmeta["dlq.last_error"]); err != nil { + if err := failBatch(ctx, store, c.registry, c.logger, bid.ID, dmeta["dlq.last_error"]); err != nil { metrics.NamedCounter(c.metricsScope, opName, "reconcile_errors", 1) return err } diff --git a/submitqueue/orchestrator/controller/dlq/batch_test.go b/submitqueue/orchestrator/controller/dlq/batch_test.go index 92c5c5d7..a125271d 100644 --- a/submitqueue/orchestrator/controller/dlq/batch_test.go +++ b/submitqueue/orchestrator/controller/dlq/batch_test.go @@ -33,7 +33,7 @@ func TestDLQBatchController_InterfaceAndAccessors(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - c := NewDLQBatchController(zaptest.NewLogger(t).Sugar(), testScope(), store, consumer.TopicRegistry{}, TopicKey(topickey.TopicKeyMerge), "orchestrator-merge-dlq") + c := NewDLQBatchController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, TopicKey(topickey.TopicKeyMerge), "orchestrator-merge-dlq") assert.Equal(t, "submitqueue-merge_dlq", c.Name()) assert.Equal(t, consumer.TopicKey("submitqueue-merge_dlq"), c.TopicKey()) @@ -67,7 +67,7 @@ func TestDLQBatchController_Process_FailsAndFansOut(t *testing.T) { store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() - c := NewDLQBatchController(zaptest.NewLogger(t).Sugar(), testScope(), store, registry, TopicKey(topickey.TopicKeyMerge), "orchestrator-merge-dlq") + c := NewDLQBatchController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, registry, TopicKey(topickey.TopicKeyMerge), "orchestrator-merge-dlq") payload, err := entity.BatchID{ID: "q/batch/9"}.ToBytes() require.NoError(t, err) @@ -81,7 +81,7 @@ func TestDLQBatchController_Process_MalformedPayloadFails(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - c := NewDLQBatchController(zaptest.NewLogger(t).Sugar(), testScope(), store, consumer.TopicRegistry{}, TopicKey(topickey.TopicKeyMerge), "orchestrator-merge-dlq") + c := NewDLQBatchController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, TopicKey(topickey.TopicKeyMerge), "orchestrator-merge-dlq") delivery := newMockDelivery(ctrl, []byte("garbage")) err := c.Process(context.Background(), delivery) @@ -93,7 +93,7 @@ func TestDLQBatchController_Process_EmptyIDFails(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - c := NewDLQBatchController(zaptest.NewLogger(t).Sugar(), testScope(), store, consumer.TopicRegistry{}, TopicKey(topickey.TopicKeyMerge), "orchestrator-merge-dlq") + c := NewDLQBatchController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, TopicKey(topickey.TopicKeyMerge), "orchestrator-merge-dlq") payload, err := entity.BatchID{ID: ""}.ToBytes() require.NoError(t, err) diff --git a/submitqueue/orchestrator/controller/dlq/buildsignal.go b/submitqueue/orchestrator/controller/dlq/buildsignal.go index b45c2255..31ffe734 100644 --- a/submitqueue/orchestrator/controller/dlq/buildsignal.go +++ b/submitqueue/orchestrator/controller/dlq/buildsignal.go @@ -38,7 +38,7 @@ import ( type buildSignalController struct { logger *zap.SugaredLogger metricsScope tally.Scope - store storage.Storage + stores storage.Factory registry consumer.TopicRegistry topicKey consumer.TopicKey consumerGroup string @@ -51,7 +51,7 @@ var _ consumer.Controller = (*buildSignalController)(nil) func NewDLQBuildSignalController( logger *zap.SugaredLogger, scope tally.Scope, - store storage.Storage, + stores storage.Factory, registry consumer.TopicRegistry, topicKey consumer.TopicKey, consumerGroup string, @@ -60,7 +60,7 @@ func NewDLQBuildSignalController( return &buildSignalController{ logger: logger.Named(name), metricsScope: scope.SubScope(name), - store: store, + stores: stores, registry: registry, topicKey: topicKey, consumerGroup: consumerGroup, @@ -83,6 +83,13 @@ func (c *buildSignalController) Process(ctx context.Context, delivery consumer.D return fmt.Errorf("dlq payload decoded to empty build id") } + store, err := c.stores.For(storage.Config{QueueName: bid.Queue}) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "storage_resolve_errors", 1) + // Non-retryable: a missing or unresolvable queue is a malformed message. + return fmt.Errorf("failed to resolve storage for queue %q: %w", bid.Queue, err) + } + dmeta := delivery.Metadata() c.logger.Warnw("dlq message received", "build_id", bid.ID, @@ -92,7 +99,7 @@ func (c *buildSignalController) Process(ctx context.Context, delivery consumer.D "dlq_last_error", dmeta["dlq.last_error"], ) - build, err := c.store.GetBuildStore().Get(ctx, bid.ID) + build, err := store.GetBuildStore().Get(ctx, bid.ID) if err != nil { if errors.Is(err, storage.ErrNotFound) { // The build was never persisted (e.g. the build controller crashed @@ -118,7 +125,7 @@ func (c *buildSignalController) Process(ctx context.Context, delivery consumer.D return nil } - if err := failBatch(ctx, c.store, c.registry, c.logger, build.BatchID, dmeta["dlq.last_error"]); err != nil { + if err := failBatch(ctx, store, c.registry, c.logger, build.BatchID, dmeta["dlq.last_error"]); err != nil { metrics.NamedCounter(c.metricsScope, opName, "reconcile_errors", 1) return err } diff --git a/submitqueue/orchestrator/controller/dlq/buildsignal_test.go b/submitqueue/orchestrator/controller/dlq/buildsignal_test.go index fa8c12e2..9f1be5ab 100644 --- a/submitqueue/orchestrator/controller/dlq/buildsignal_test.go +++ b/submitqueue/orchestrator/controller/dlq/buildsignal_test.go @@ -34,7 +34,7 @@ func TestDLQBuildSignalController_InterfaceAndAccessors(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - c := NewDLQBuildSignalController(zaptest.NewLogger(t).Sugar(), testScope(), store, consumer.TopicRegistry{}, TopicKey(topickey.TopicKeyBuildSignal), "orchestrator-buildsignal-dlq") + c := NewDLQBuildSignalController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, TopicKey(topickey.TopicKeyBuildSignal), "orchestrator-buildsignal-dlq") assert.Equal(t, "buildsignal_dlq", c.Name()) assert.Equal(t, consumer.TopicKey("buildsignal_dlq"), c.TopicKey()) @@ -74,7 +74,7 @@ func TestDLQBuildSignalController_Process_FansOutToBatch(t *testing.T) { store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() - c := NewDLQBuildSignalController(zaptest.NewLogger(t).Sugar(), testScope(), store, registry, TopicKey(topickey.TopicKeyBuildSignal), "orchestrator-buildsignal-dlq") + c := NewDLQBuildSignalController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, registry, TopicKey(topickey.TopicKeyBuildSignal), "orchestrator-buildsignal-dlq") payload, err := entity.BuildID{ID: "build-1"}.ToBytes() require.NoError(t, err) @@ -93,7 +93,7 @@ func TestDLQBuildSignalController_Process_BuildNotFoundIsNoOp(t *testing.T) { store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() store.EXPECT().GetBuildStore().Return(buildStore).AnyTimes() - c := NewDLQBuildSignalController(zaptest.NewLogger(t).Sugar(), testScope(), store, consumer.TopicRegistry{}, TopicKey(topickey.TopicKeyBuildSignal), "orchestrator-buildsignal-dlq") + c := NewDLQBuildSignalController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, TopicKey(topickey.TopicKeyBuildSignal), "orchestrator-buildsignal-dlq") payload, err := entity.BuildID{ID: "build-1"}.ToBytes() require.NoError(t, err) @@ -114,7 +114,7 @@ func TestDLQBuildSignalController_Process_BuildMissingBatchIsNoOp(t *testing.T) store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() store.EXPECT().GetBuildStore().Return(buildStore).AnyTimes() - c := NewDLQBuildSignalController(zaptest.NewLogger(t).Sugar(), testScope(), store, consumer.TopicRegistry{}, TopicKey(topickey.TopicKeyBuildSignal), "orchestrator-buildsignal-dlq") + c := NewDLQBuildSignalController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, TopicKey(topickey.TopicKeyBuildSignal), "orchestrator-buildsignal-dlq") payload, err := entity.BuildID{ID: "build-1"}.ToBytes() require.NoError(t, err) @@ -128,7 +128,7 @@ func TestDLQBuildSignalController_Process_MalformedPayloadFails(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - c := NewDLQBuildSignalController(zaptest.NewLogger(t).Sugar(), testScope(), store, consumer.TopicRegistry{}, TopicKey(topickey.TopicKeyBuildSignal), "orchestrator-buildsignal-dlq") + c := NewDLQBuildSignalController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, TopicKey(topickey.TopicKeyBuildSignal), "orchestrator-buildsignal-dlq") delivery := newMockDelivery(ctrl, []byte("garbage")) err := c.Process(context.Background(), delivery) diff --git a/submitqueue/orchestrator/controller/dlq/dlq_test.go b/submitqueue/orchestrator/controller/dlq/dlq_test.go index 11d0888a..e673d7cc 100644 --- a/submitqueue/orchestrator/controller/dlq/dlq_test.go +++ b/submitqueue/orchestrator/controller/dlq/dlq_test.go @@ -33,10 +33,16 @@ import ( // newQueueBatchStateStore returns a QueueBatchStateStore mock that accepts any // membership-record write; these tests never list record buckets. +// staticStorageFactory resolves every queue to one fixed store aggregate. +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 } + func newQueueBatchStateStore(ctrl *gomock.Controller) *storagemock.MockQueueBatchStateStore { s := storagemock.NewMockQueueBatchStateStore(ctrl) s.EXPECT().Put(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - s.EXPECT().Delete(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + s.EXPECT().Delete(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() return s } diff --git a/submitqueue/orchestrator/controller/dlq/mergeconflictsignal.go b/submitqueue/orchestrator/controller/dlq/mergeconflictsignal.go index 34d922d7..e57f1431 100644 --- a/submitqueue/orchestrator/controller/dlq/mergeconflictsignal.go +++ b/submitqueue/orchestrator/controller/dlq/mergeconflictsignal.go @@ -33,7 +33,7 @@ import ( type mergeConflictSignalController struct { logger *zap.SugaredLogger metricsScope tally.Scope - store storage.Storage + stores storage.Factory registry consumer.TopicRegistry topicKey consumer.TopicKey consumerGroup string @@ -47,7 +47,7 @@ var _ consumer.Controller = (*mergeConflictSignalController)(nil) func NewDLQMergeConflictSignalController( logger *zap.SugaredLogger, scope tally.Scope, - store storage.Storage, + stores storage.Factory, registry consumer.TopicRegistry, topicKey consumer.TopicKey, consumerGroup string, @@ -56,7 +56,7 @@ func NewDLQMergeConflictSignalController( return &mergeConflictSignalController{ logger: logger.Named(name), metricsScope: scope.SubScope(name), - store: store, + stores: stores, registry: registry, topicKey: topicKey, consumerGroup: consumerGroup, @@ -75,6 +75,13 @@ func (c *mergeConflictSignalController) Process(ctx context.Context, delivery co return fmt.Errorf("failed to decode merge conflict check result from dlq payload: %w", err) } + store, err := c.stores.For(storage.Config{QueueName: result.GetQueueName()}) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "storage_resolve_errors", 1) + // Non-retryable: a missing or unresolvable queue is a malformed message. + return fmt.Errorf("failed to resolve storage for queue %q: %w", result.GetQueueName(), err) + } + dmeta := delivery.Metadata() c.logger.Warnw("dlq message received", "request_id", result.Id, @@ -84,7 +91,7 @@ func (c *mergeConflictSignalController) Process(ctx context.Context, delivery co "dlq_last_error", dmeta["dlq.last_error"], ) - if err := failRequest(ctx, c.store, c.registry, c.logger, result.Id, dmeta["dlq.last_error"]); err != nil { + if err := failRequest(ctx, store, c.registry, c.logger, result.Id, dmeta["dlq.last_error"]); err != nil { metrics.NamedCounter(c.metricsScope, opName, "reconcile_errors", 1) return err } diff --git a/submitqueue/orchestrator/controller/dlq/mergeconflictsignal_test.go b/submitqueue/orchestrator/controller/dlq/mergeconflictsignal_test.go index d0aeb10e..bf23a1d6 100644 --- a/submitqueue/orchestrator/controller/dlq/mergeconflictsignal_test.go +++ b/submitqueue/orchestrator/controller/dlq/mergeconflictsignal_test.go @@ -34,7 +34,7 @@ func TestDLQMergeConflictSignalController_InterfaceAndAccessors(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - c := NewDLQMergeConflictSignalController(zaptest.NewLogger(t).Sugar(), testScope(), store, consumer.TopicRegistry{}, TopicKey(runwaymq.TopicKeyMergeConflictCheckSignal), "orchestrator-mergeconflictsignal-dlq") + c := NewDLQMergeConflictSignalController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, TopicKey(runwaymq.TopicKeyMergeConflictCheckSignal), "orchestrator-mergeconflictsignal-dlq") assert.Equal(t, "merge-conflict-check-signal_dlq", c.Name()) assert.Equal(t, consumer.TopicKey("merge-conflict-check-signal_dlq"), c.TopicKey()) @@ -59,7 +59,7 @@ func TestDLQMergeConflictSignalController_Process_ReconcilesRequest(t *testing.T store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() - c := NewDLQMergeConflictSignalController(zaptest.NewLogger(t).Sugar(), testScope(), store, registry, TopicKey(runwaymq.TopicKeyMergeConflictCheckSignal), "orchestrator-mergeconflictsignal-dlq") + c := NewDLQMergeConflictSignalController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, registry, TopicKey(runwaymq.TopicKeyMergeConflictCheckSignal), "orchestrator-mergeconflictsignal-dlq") payload, err := runwaymq.Marshal(&runwaymq.MergeResult{Id: "q/1", Outcome: runwaypb.Outcome_FAILED, Reason: "boom"}) require.NoError(t, err) @@ -73,7 +73,7 @@ func TestDLQMergeConflictSignalController_Process_MalformedPayloadFails(t *testi store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - c := NewDLQMergeConflictSignalController(zaptest.NewLogger(t).Sugar(), testScope(), store, consumer.TopicRegistry{}, TopicKey(runwaymq.TopicKeyMergeConflictCheckSignal), "orchestrator-mergeconflictsignal-dlq") + c := NewDLQMergeConflictSignalController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, TopicKey(runwaymq.TopicKeyMergeConflictCheckSignal), "orchestrator-mergeconflictsignal-dlq") delivery := newMockDelivery(ctrl, []byte("garbage")) require.Error(t, c.Process(context.Background(), delivery)) diff --git a/submitqueue/orchestrator/controller/dlq/mergesignal.go b/submitqueue/orchestrator/controller/dlq/mergesignal.go index 1eb5aec7..4243df8b 100644 --- a/submitqueue/orchestrator/controller/dlq/mergesignal.go +++ b/submitqueue/orchestrator/controller/dlq/mergesignal.go @@ -33,7 +33,7 @@ import ( type mergeSignalController struct { logger *zap.SugaredLogger metricsScope tally.Scope - store storage.Storage + stores storage.Factory registry consumer.TopicRegistry topicKey consumer.TopicKey consumerGroup string @@ -46,7 +46,7 @@ var _ consumer.Controller = (*mergeSignalController)(nil) func NewDLQMergeSignalController( logger *zap.SugaredLogger, scope tally.Scope, - store storage.Storage, + stores storage.Factory, registry consumer.TopicRegistry, topicKey consumer.TopicKey, consumerGroup string, @@ -55,7 +55,7 @@ func NewDLQMergeSignalController( return &mergeSignalController{ logger: logger.Named(name), metricsScope: scope.SubScope(name), - store: store, + stores: stores, registry: registry, topicKey: topicKey, consumerGroup: consumerGroup, @@ -74,6 +74,13 @@ func (c *mergeSignalController) Process(ctx context.Context, delivery consumer.D return fmt.Errorf("failed to decode merge result from dlq payload: %w", err) } + store, err := c.stores.For(storage.Config{QueueName: result.GetQueueName()}) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "storage_resolve_errors", 1) + // Non-retryable: a missing or unresolvable queue is a malformed message. + return fmt.Errorf("failed to resolve storage for queue %q: %w", result.GetQueueName(), err) + } + dmeta := delivery.Metadata() c.logger.Warnw("dlq message received", "batch_id", result.Id, @@ -83,7 +90,7 @@ func (c *mergeSignalController) Process(ctx context.Context, delivery consumer.D "dlq_last_error", dmeta["dlq.last_error"], ) - if err := failBatch(ctx, c.store, c.registry, c.logger, result.Id, dmeta["dlq.last_error"]); err != nil { + if err := failBatch(ctx, store, c.registry, c.logger, result.Id, dmeta["dlq.last_error"]); err != nil { metrics.NamedCounter(c.metricsScope, opName, "reconcile_errors", 1) return err } diff --git a/submitqueue/orchestrator/controller/dlq/mergesignal_test.go b/submitqueue/orchestrator/controller/dlq/mergesignal_test.go index a33b034d..69a122ae 100644 --- a/submitqueue/orchestrator/controller/dlq/mergesignal_test.go +++ b/submitqueue/orchestrator/controller/dlq/mergesignal_test.go @@ -34,7 +34,7 @@ func TestDLQMergeSignalController_InterfaceAndAccessors(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - c := NewDLQMergeSignalController(zaptest.NewLogger(t).Sugar(), testScope(), store, consumer.TopicRegistry{}, TopicKey(runwaymq.TopicKeyMergeSignal), "orchestrator-mergesignal-dlq") + c := NewDLQMergeSignalController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, TopicKey(runwaymq.TopicKeyMergeSignal), "orchestrator-mergesignal-dlq") assert.Equal(t, "merge-signal_dlq", c.Name()) assert.Equal(t, consumer.TopicKey("merge-signal_dlq"), c.TopicKey()) @@ -70,7 +70,7 @@ func TestDLQMergeSignalController_Process_ReconcilesBatch(t *testing.T) { store.EXPECT().GetBatchStore().Return(batchStore).AnyTimes() store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() - c := NewDLQMergeSignalController(zaptest.NewLogger(t).Sugar(), testScope(), store, registry, TopicKey(runwaymq.TopicKeyMergeSignal), "orchestrator-mergesignal-dlq") + c := NewDLQMergeSignalController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, registry, TopicKey(runwaymq.TopicKeyMergeSignal), "orchestrator-mergesignal-dlq") payload, err := runwaymq.Marshal(&runwaymq.MergeResult{Id: "q/batch/1", Outcome: runwaypb.Outcome_FAILED, Reason: "boom"}) require.NoError(t, err) @@ -84,7 +84,7 @@ func TestDLQMergeSignalController_Process_MalformedPayloadFails(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - c := NewDLQMergeSignalController(zaptest.NewLogger(t).Sugar(), testScope(), store, consumer.TopicRegistry{}, TopicKey(runwaymq.TopicKeyMergeSignal), "orchestrator-mergesignal-dlq") + c := NewDLQMergeSignalController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, TopicKey(runwaymq.TopicKeyMergeSignal), "orchestrator-mergesignal-dlq") delivery := newMockDelivery(ctrl, []byte("garbage")) require.Error(t, c.Process(context.Background(), delivery)) diff --git a/submitqueue/orchestrator/controller/dlq/request.go b/submitqueue/orchestrator/controller/dlq/request.go index 615346c4..f5fafec9 100644 --- a/submitqueue/orchestrator/controller/dlq/request.go +++ b/submitqueue/orchestrator/controller/dlq/request.go @@ -26,41 +26,38 @@ import ( "go.uber.org/zap" ) -// RequestIDDecoder extracts the affected request ID from the raw payload bytes -// of a DLQ message. Different primary topics carry different payload shapes -// (LandRequest on start, CancelRequest on cancel, RequestID on validate / -// batch), so the caller injects the right decoder for the topic being -// reconciled. Returning an empty ID is treated as a decode failure. -type RequestIDDecoder func(payload []byte) (string, error) +// RequestIDDecoder extracts the affected request's identity — its ID and its +// queue — from the raw payload bytes of a DLQ message. Different primary +// topics carry different payload shapes (LandRequest on start, CancelRequest +// on cancel, RequestID on validate / batch), so the caller injects the right +// decoder for the topic being reconciled. Returning an empty ID is treated as +// a decode failure. +type RequestIDDecoder func(payload []byte) (entity.RequestID, error) // DecodeLandRequestID extracts the request ID from a LandRequest payload // (the shape used by the start topic). -func DecodeLandRequestID(payload []byte) (string, error) { +func DecodeLandRequestID(payload []byte) (entity.RequestID, error) { lr, err := entity.LandRequestFromBytes(payload) if err != nil { - return "", err + return entity.RequestID{}, err } - return lr.ID, nil + return entity.RequestID{ID: lr.ID, Queue: lr.Queue}, nil } // DecodeCancelRequestID extracts the request ID from a CancelRequest payload // (the shape used by the cancel topic). -func DecodeCancelRequestID(payload []byte) (string, error) { +func DecodeCancelRequestID(payload []byte) (entity.RequestID, error) { cr, err := entity.CancelRequestFromBytes(payload) if err != nil { - return "", err + return entity.RequestID{}, err } - return cr.ID, nil + return entity.RequestID{ID: cr.ID, Queue: cr.Queue}, nil } // DecodeRequestID extracts the request ID from a RequestID payload (the shape // used by the validate and batch topics). -func DecodeRequestID(payload []byte) (string, error) { - rid, err := entity.RequestIDFromBytes(payload) - if err != nil { - return "", err - } - return rid.ID, nil +func DecodeRequestID(payload []byte) (entity.RequestID, error) { + return entity.RequestIDFromBytes(payload) } // requestController is the DLQ reconciler for request-scoped pipeline stages. @@ -71,7 +68,7 @@ func DecodeRequestID(payload []byte) (string, error) { type requestController struct { logger *zap.SugaredLogger metricsScope tally.Scope - store storage.Storage + stores storage.Factory registry consumer.TopicRegistry decode RequestIDDecoder topicKey consumer.TopicKey @@ -87,7 +84,7 @@ var _ consumer.Controller = (*requestController)(nil) func NewDLQRequestController( logger *zap.SugaredLogger, scope tally.Scope, - store storage.Storage, + stores storage.Factory, registry consumer.TopicRegistry, decode RequestIDDecoder, topicKey consumer.TopicKey, @@ -97,7 +94,7 @@ func NewDLQRequestController( return &requestController{ logger: logger.Named(name), metricsScope: scope.SubScope(name), - store: store, + stores: stores, registry: registry, decode: decode, topicKey: topicKey, @@ -111,7 +108,7 @@ func (c *requestController) Process(ctx context.Context, delivery consumer.Deliv msg := delivery.Message() - requestID, err := c.decode(msg.Payload) + rid, err := c.decode(msg.Payload) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "deserialize_errors", 1) // Malformed DLQ payload is non-retryable: a re-delivery will decode the @@ -120,21 +117,28 @@ func (c *requestController) Process(ctx context.Context, delivery consumer.Deliv // acked and dropped after the error is logged. return fmt.Errorf("failed to decode dlq payload: %w", err) } - if requestID == "" { + if rid.ID == "" { metrics.NamedCounter(c.metricsScope, opName, "empty_id_errors", 1) return fmt.Errorf("dlq payload decoded to empty request id") } + store, err := c.stores.For(storage.Config{QueueName: rid.Queue}) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "storage_resolve_errors", 1) + // Non-retryable: a missing or unresolvable queue is a malformed message. + return fmt.Errorf("failed to resolve storage for queue %q: %w", rid.Queue, err) + } + dmeta := delivery.Metadata() c.logger.Warnw("dlq message received", - "request_id", requestID, + "request_id", rid.ID, "attempt", delivery.Attempt(), "dlq_original_topic", dmeta["dlq.original_topic"], "dlq_failure_count", dmeta["dlq.failure_count"], "dlq_last_error", dmeta["dlq.last_error"], ) - if err := failRequest(ctx, c.store, c.registry, c.logger, requestID, dmeta["dlq.last_error"]); err != nil { + if err := failRequest(ctx, store, c.registry, c.logger, rid.ID, dmeta["dlq.last_error"]); err != nil { metrics.NamedCounter(c.metricsScope, opName, "reconcile_errors", 1) return err } diff --git a/submitqueue/orchestrator/controller/dlq/request_test.go b/submitqueue/orchestrator/controller/dlq/request_test.go index 2d7b264b..4b317d97 100644 --- a/submitqueue/orchestrator/controller/dlq/request_test.go +++ b/submitqueue/orchestrator/controller/dlq/request_test.go @@ -35,7 +35,7 @@ func TestDLQRequestController_InterfaceAndAccessors(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() - c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), store, consumer.TopicRegistry{}, DecodeRequestID, TopicKey(topickey.TopicKeyValidate), "orchestrator-validate-dlq") + c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, DecodeRequestID, TopicKey(topickey.TopicKeyValidate), "orchestrator-validate-dlq") assert.Equal(t, "validate_dlq", c.Name()) assert.Equal(t, consumer.TopicKey("validate_dlq"), c.TopicKey()) @@ -60,7 +60,7 @@ func TestDLQRequestController_Process_LandRequestPayload(t *testing.T) { store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() - c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), store, registry, DecodeLandRequestID, TopicKey(topickey.TopicKeyStart), "orchestrator-start-dlq") + c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, registry, DecodeLandRequestID, TopicKey(topickey.TopicKeyStart), "orchestrator-start-dlq") payload, err := entity.LandRequest{ID: "q/1", Queue: "q"}.ToBytes() require.NoError(t, err) @@ -87,7 +87,7 @@ func TestDLQRequestController_Process_CancelRequestPayload(t *testing.T) { store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() - c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), store, registry, DecodeCancelRequestID, TopicKey(topickey.TopicKeyCancel), "orchestrator-cancel-dlq") + c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, registry, DecodeCancelRequestID, TopicKey(topickey.TopicKeyCancel), "orchestrator-cancel-dlq") payload, err := entity.CancelRequest{ID: "q/7", Reason: "user"}.ToBytes() require.NoError(t, err) @@ -115,7 +115,7 @@ func TestDLQRequestController_Process_RequestIDPayload(t *testing.T) { store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() - c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), store, registry, DecodeRequestID, TopicKey(topickey.TopicKeyBatch), "orchestrator-batch-dlq") + c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, registry, DecodeRequestID, TopicKey(topickey.TopicKeyBatch), "orchestrator-batch-dlq") payload, err := entity.RequestID{ID: "q/3"}.ToBytes() require.NoError(t, err) @@ -136,7 +136,7 @@ func TestDLQRequestController_Process_DifferentTerminalOutcomeSkips(t *testing.T store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() store.EXPECT().GetRequestStore().Return(requestStore).AnyTimes() - c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), store, consumer.TopicRegistry{}, DecodeRequestID, TopicKey(topickey.TopicKeyValidate), "orchestrator-validate-dlq") + c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, DecodeRequestID, TopicKey(topickey.TopicKeyValidate), "orchestrator-validate-dlq") payload, err := entity.RequestID{ID: "q/1"}.ToBytes() require.NoError(t, err) @@ -152,7 +152,7 @@ func TestDLQRequestController_Process_MalformedPayloadFails(t *testing.T) { store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() // no store calls expected - c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), store, consumer.TopicRegistry{}, DecodeRequestID, TopicKey(topickey.TopicKeyValidate), "orchestrator-validate-dlq") + c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, DecodeRequestID, TopicKey(topickey.TopicKeyValidate), "orchestrator-validate-dlq") delivery := newMockDelivery(ctrl, []byte("not json")) err := c.Process(context.Background(), delivery) @@ -166,7 +166,7 @@ func TestDLQRequestController_Process_EmptyIDFails(t *testing.T) { store.EXPECT().GetQueueBatchStateStore().Return(newQueueBatchStateStore(ctrl)).AnyTimes() // no store calls expected - c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), store, consumer.TopicRegistry{}, DecodeRequestID, TopicKey(topickey.TopicKeyValidate), "orchestrator-validate-dlq") + c := NewDLQRequestController(zaptest.NewLogger(t).Sugar(), testScope(), staticStorageFactory{store: store}, consumer.TopicRegistry{}, DecodeRequestID, TopicKey(topickey.TopicKeyValidate), "orchestrator-validate-dlq") payload, err := entity.RequestID{ID: ""}.ToBytes() require.NoError(t, err) diff --git a/submitqueue/orchestrator/controller/merge/BUILD.bazel b/submitqueue/orchestrator/controller/merge/BUILD.bazel index d62738ee..61aceedc 100644 --- a/submitqueue/orchestrator/controller/merge/BUILD.bazel +++ b/submitqueue/orchestrator/controller/merge/BUILD.bazel @@ -36,6 +36,7 @@ go_test( "//platform/extension/messagequeue/mock:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", + "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mock:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", diff --git a/submitqueue/orchestrator/controller/merge/merge.go b/submitqueue/orchestrator/controller/merge/merge.go index 484ba9c5..c2d5d8b1 100644 --- a/submitqueue/orchestrator/controller/merge/merge.go +++ b/submitqueue/orchestrator/controller/merge/merge.go @@ -50,7 +50,7 @@ import ( type Controller struct { logger *zap.SugaredLogger metricsScope tally.Scope - store storage.Storage + stores storage.Factory registry consumer.TopicRegistry runwayTopicKey consumer.TopicKey topicKey consumer.TopicKey @@ -66,7 +66,7 @@ var _ consumer.Controller = (*Controller)(nil) func NewController( logger *zap.SugaredLogger, scope tally.Scope, - store storage.Storage, + stores storage.Factory, registry consumer.TopicRegistry, runwayTopicKey consumer.TopicKey, topicKey consumer.TopicKey, @@ -75,7 +75,7 @@ func NewController( return &Controller{ logger: logger.Named("merge_controller"), metricsScope: scope.SubScope("merge_controller"), - store: store, + stores: stores, registry: registry, runwayTopicKey: runwayTopicKey, topicKey: topicKey, @@ -101,7 +101,14 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er return fmt.Errorf("failed to deserialize batch ID: %w", err) } - batch, err := c.store.GetBatchStore().Get(ctx, bid.ID) + store, err := c.stores.For(storage.Config{QueueName: bid.Queue}) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "storage_resolve_errors", 1) + // Non-retryable: a missing or unresolvable queue is a malformed message. + return fmt.Errorf("failed to resolve storage for queue %q: %w", bid.Queue, err) + } + + batch, err := store.GetBatchStore().Get(ctx, bid.ID) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to get batch %s: %w", bid.ID, err) @@ -140,7 +147,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // Build the full payload runway needs to perform the merge. The batch id is // the client-owned correlation id, so a redelivery republishes the same id // and runway dedupes on it; the result is matched straight back to the batch. - req, err := c.buildMergeRequest(ctx, batch) + req, err := c.buildMergeRequest(ctx, store, batch) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to build merge request for batch %s: %w", batch.ID, err) @@ -163,10 +170,10 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // buildMergeRequest loads the batch's member requests and assembles the runway // merge request: one MergeStep per request, in Contains order, attributed by // request id and carrying that request's change and land strategy. -func (c *Controller) buildMergeRequest(ctx context.Context, batch entity.Batch) (*runwaymq.MergeRequest, error) { +func (c *Controller) buildMergeRequest(ctx context.Context, store storage.Storage, batch entity.Batch) (*runwaymq.MergeRequest, error) { steps := make([]*runwaymq.MergeStep, 0, len(batch.Contains)) for _, requestID := range batch.Contains { - request, err := c.store.GetRequestStore().Get(ctx, requestID) + request, err := store.GetRequestStore().Get(ctx, requestID) if err != nil { return nil, fmt.Errorf("failed to get request %s: %w", requestID, err) } diff --git a/submitqueue/orchestrator/controller/merge/merge_test.go b/submitqueue/orchestrator/controller/merge/merge_test.go index 81212c31..92a3fb1b 100644 --- a/submitqueue/orchestrator/controller/merge/merge_test.go +++ b/submitqueue/orchestrator/controller/merge/merge_test.go @@ -36,10 +36,17 @@ import ( queuemock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" "github.com/uber/submitqueue/submitqueue/core/topickey" "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" ) // batchIDPayload serializes a BatchID to JSON bytes for test message payloads. +// staticStorageFactory resolves every queue to one fixed store aggregate. +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 } + func batchIDPayload(t *testing.T, id string) []byte { payload, err := entity.BatchID{ID: id}.ToBytes() require.NoError(t, err) @@ -58,7 +65,7 @@ func newController(t *testing.T, store *storagemock.MockStorage, registry consum return NewController( zaptest.NewLogger(t).Sugar(), tally.NoopScope, - store, + staticStorageFactory{store: store}, registry, runwaymq.TopicKeyMerge, topickey.TopicKeyMerge, diff --git a/submitqueue/orchestrator/controller/mergeconflictsignal/BUILD.bazel b/submitqueue/orchestrator/controller/mergeconflictsignal/BUILD.bazel index 0d3b668a..07ac5501 100644 --- a/submitqueue/orchestrator/controller/mergeconflictsignal/BUILD.bazel +++ b/submitqueue/orchestrator/controller/mergeconflictsignal/BUILD.bazel @@ -33,6 +33,7 @@ go_test( "//platform/extension/messagequeue/mock:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", + "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mock:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", diff --git a/submitqueue/orchestrator/controller/mergeconflictsignal/mergeconflictsignal.go b/submitqueue/orchestrator/controller/mergeconflictsignal/mergeconflictsignal.go index 761957b1..8819773a 100644 --- a/submitqueue/orchestrator/controller/mergeconflictsignal/mergeconflictsignal.go +++ b/submitqueue/orchestrator/controller/mergeconflictsignal/mergeconflictsignal.go @@ -40,7 +40,7 @@ import ( type Controller struct { logger *zap.SugaredLogger metricsScope tally.Scope - store storage.Storage + stores storage.Factory registry consumer.TopicRegistry topicKey consumer.TopicKey consumerGroup string @@ -53,7 +53,7 @@ var _ consumer.Controller = (*Controller)(nil) func NewController( logger *zap.SugaredLogger, scope tally.Scope, - store storage.Storage, + stores storage.Factory, registry consumer.TopicRegistry, topicKey consumer.TopicKey, consumerGroup string, @@ -61,7 +61,7 @@ func NewController( return &Controller{ logger: logger.Named("mergeconflictsignal_controller"), metricsScope: scope.SubScope("mergeconflictsignal_controller"), - store: store, + stores: stores, registry: registry, topicKey: topicKey, consumerGroup: consumerGroup, @@ -89,7 +89,14 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er return fmt.Errorf("failed to deserialize merge conflict check result: %w", err) } - request, err := c.store.GetRequestStore().Get(ctx, result.Id) + store, err := c.stores.For(storage.Config{QueueName: result.GetQueueName()}) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "storage_resolve_errors", 1) + // Non-retryable: a missing or unresolvable queue is a malformed message. + return fmt.Errorf("failed to resolve storage for queue %q: %w", result.GetQueueName(), err) + } + + request, err := store.GetRequestStore().Get(ctx, result.Id) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to get request %s: %w", result.Id, err) @@ -118,7 +125,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er "request_id", request.ID, "reason", result.Reason, ) - if err := c.failRequest(ctx, request, result.Reason); err != nil { + if err := c.failRequest(ctx, store, request, result.Reason); err != nil { metrics.NamedCounter(c.metricsScope, opName, "fail_errors", 1) return fmt.Errorf("failed to fail request %s: %w", request.ID, err) } @@ -128,7 +135,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // Advance the request to Validated now that the merge-conflict check passed. newVersion := request.Version + 1 request.State = entity.RequestStateValidated - if err := c.store.GetRequestStore().Update(ctx, request, request.Version, newVersion); err != nil { + if err := store.GetRequestStore().Update(ctx, request, request.Version, newVersion); err != nil { metrics.NamedCounter(c.metricsScope, opName, "state_errors", 1) return fmt.Errorf("failed to update request %s state to validated: %w", request.ID, err) } @@ -161,7 +168,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // in Error skips the state CAS but still publishes the log (so a prior attempt // that flipped the state but failed before logging is repaired); a request that // reached a different terminal state (e.g. a racing cancel) is left untouched. -func (c *Controller) failRequest(ctx context.Context, request entity.Request, reason string) error { +func (c *Controller) failRequest(ctx context.Context, store storage.Storage, request entity.Request, reason string) error { switch { case request.State == entity.RequestStateError: // Idempotent retry: a prior delivery already wrote Error. Fall through to @@ -175,7 +182,7 @@ func (c *Controller) failRequest(ctx context.Context, request entity.Request, re default: newVersion := request.Version + 1 request.State = entity.RequestStateError - if err := c.store.GetRequestStore().Update(ctx, request, request.Version, newVersion); err != nil { + if err := store.GetRequestStore().Update(ctx, request, request.Version, newVersion); err != nil { return fmt.Errorf("failed to update request %s state to error: %w", request.ID, err) } request.Version = newVersion diff --git a/submitqueue/orchestrator/controller/mergeconflictsignal/mergeconflictsignal_test.go b/submitqueue/orchestrator/controller/mergeconflictsignal/mergeconflictsignal_test.go index da3fcd96..c71dd9fb 100644 --- a/submitqueue/orchestrator/controller/mergeconflictsignal/mergeconflictsignal_test.go +++ b/submitqueue/orchestrator/controller/mergeconflictsignal/mergeconflictsignal_test.go @@ -30,11 +30,18 @@ import ( queuemock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" "github.com/uber/submitqueue/submitqueue/core/topickey" "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" "go.uber.org/mock/gomock" "go.uber.org/zap/zaptest" ) +// staticStorageFactory resolves every queue to one fixed store aggregate. +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 } + func requestWithState(request entity.Request, state entity.RequestState) entity.Request { request.State = state return request @@ -89,7 +96,7 @@ func TestProcess_MergeablePublishesToBatch(t *testing.T) { ) require.NoError(t, err) - controller := NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, store, registry, + controller := NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, staticStorageFactory{store: store}, registry, runwaymq.TopicKeyMergeConflictCheckSignal, "orchestrator-mergeconflictsignal") res := runwaymq.MergeResult{Id: testRequestID, Outcome: runwaypb.Outcome_SUCCEEDED} @@ -146,7 +153,7 @@ func TestProcess_NotMergeableMarksRequestError(t *testing.T) { ) require.NoError(t, err) - controller := NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, store, registry, + controller := NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, staticStorageFactory{store: store}, registry, runwaymq.TopicKeyMergeConflictCheckSignal, "orchestrator-mergeconflictsignal") res := runwaymq.MergeResult{Id: testRequestID, Outcome: runwaypb.Outcome_FAILED, Reason: "conflict in foo.go"} @@ -173,10 +180,10 @@ func TestFailRequest_UpdateFailureLeavesRequestUnchanged(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().GetRequestStore().Return(reqStore) - controller := NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, store, consumer.TopicRegistry{}, + controller := NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, staticStorageFactory{store: store}, consumer.TopicRegistry{}, runwaymq.TopicKeyMergeConflictCheckSignal, "orchestrator-mergeconflictsignal") - err := controller.failRequest(context.Background(), request, "conflict") + err := controller.failRequest(context.Background(), store, request, "conflict") require.Error(t, err) assert.Equal(t, entity.RequestStateStarted, request.State) assert.Equal(t, int32(1), request.Version) @@ -201,7 +208,7 @@ func TestProcess_HaltedRequestSkips(t *testing.T) { ) require.NoError(t, err) - controller := NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, store, registry, + controller := NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, staticStorageFactory{store: store}, registry, runwaymq.TopicKeyMergeConflictCheckSignal, "orchestrator-mergeconflictsignal") res := runwaymq.MergeResult{Id: testRequestID, Outcome: runwaypb.Outcome_SUCCEEDED} diff --git a/submitqueue/orchestrator/controller/mergesignal/BUILD.bazel b/submitqueue/orchestrator/controller/mergesignal/BUILD.bazel index b1fb73d9..aa85d1aa 100644 --- a/submitqueue/orchestrator/controller/mergesignal/BUILD.bazel +++ b/submitqueue/orchestrator/controller/mergesignal/BUILD.bazel @@ -33,6 +33,7 @@ go_test( "//platform/extension/messagequeue/mock:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", + "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mock:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", diff --git a/submitqueue/orchestrator/controller/mergesignal/mergesignal.go b/submitqueue/orchestrator/controller/mergesignal/mergesignal.go index 6617ba0a..6d94b7f7 100644 --- a/submitqueue/orchestrator/controller/mergesignal/mergesignal.go +++ b/submitqueue/orchestrator/controller/mergesignal/mergesignal.go @@ -42,7 +42,7 @@ import ( type Controller struct { logger *zap.SugaredLogger metricsScope tally.Scope - store storage.Storage + stores storage.Factory registry consumer.TopicRegistry topicKey consumer.TopicKey consumerGroup string @@ -55,7 +55,7 @@ var _ consumer.Controller = (*Controller)(nil) func NewController( logger *zap.SugaredLogger, scope tally.Scope, - store storage.Storage, + stores storage.Factory, registry consumer.TopicRegistry, topicKey consumer.TopicKey, consumerGroup string, @@ -63,7 +63,7 @@ func NewController( return &Controller{ logger: logger.Named("mergesignal_controller"), metricsScope: scope.SubScope("mergesignal_controller"), - store: store, + stores: stores, registry: registry, topicKey: topicKey, consumerGroup: consumerGroup, @@ -91,7 +91,14 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er return fmt.Errorf("failed to deserialize merge result: %w", err) } - batch, err := c.store.GetBatchStore().Get(ctx, result.Id) + store, err := c.stores.For(storage.Config{QueueName: result.GetQueueName()}) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "storage_resolve_errors", 1) + // Non-retryable: a missing or unresolvable queue is a malformed message. + return fmt.Errorf("failed to resolve storage for queue %q: %w", result.GetQueueName(), err) + } + + batch, err := store.GetBatchStore().Get(ctx, result.Id) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to get batch %s: %w", result.Id, err) @@ -121,7 +128,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // attempt missed the downstream publishes, then ack. if batch.State.IsTerminal() { metrics.NamedCounter(c.metricsScope, opName, "skipped_terminal", 1) - if err := corebatch.EnsureRecord(ctx, c.store, batch); err != nil { + if err := corebatch.EnsureRecord(ctx, store, batch); err != nil { metrics.NamedCounter(c.metricsScope, opName, "state_update_errors", 1) return err } @@ -144,7 +151,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er ) } - batch, err = corebatch.Transition(ctx, c.store, batch, newState) + batch, err = corebatch.Transition(ctx, store, batch, newState) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "state_update_errors", 1) return err diff --git a/submitqueue/orchestrator/controller/mergesignal/mergesignal_test.go b/submitqueue/orchestrator/controller/mergesignal/mergesignal_test.go index 4e989ec5..8655ba31 100644 --- a/submitqueue/orchestrator/controller/mergesignal/mergesignal_test.go +++ b/submitqueue/orchestrator/controller/mergesignal/mergesignal_test.go @@ -29,6 +29,7 @@ import ( queuemock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" "github.com/uber/submitqueue/submitqueue/core/topickey" "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" "go.uber.org/mock/gomock" "go.uber.org/zap/zaptest" @@ -36,10 +37,16 @@ import ( // newQueueBatchStateStore returns a QueueBatchStateStore mock that accepts any // membership-record write; these tests never list record buckets. +// staticStorageFactory resolves every queue to one fixed store aggregate. +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 } + func newQueueBatchStateStore(ctrl *gomock.Controller) *storagemock.MockQueueBatchStateStore { s := storagemock.NewMockQueueBatchStateStore(ctrl) s.EXPECT().Put(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - s.EXPECT().Delete(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + s.EXPECT().Delete(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() return s } @@ -90,7 +97,7 @@ func newController(t *testing.T, store *storagemock.MockStorage, registry consum return NewController( zaptest.NewLogger(t).Sugar(), tally.NoopScope, - store, + staticStorageFactory{store: store}, registry, runwaymq.TopicKeyMergeSignal, "orchestrator-mergesignal", diff --git a/submitqueue/orchestrator/controller/speculate/speculate.go b/submitqueue/orchestrator/controller/speculate/speculate.go index 3ed8b186..1ddb929a 100644 --- a/submitqueue/orchestrator/controller/speculate/speculate.go +++ b/submitqueue/orchestrator/controller/speculate/speculate.go @@ -57,7 +57,7 @@ import ( type Controller struct { logger *zap.SugaredLogger metricsScope tally.Scope - store storage.Storage + stores storage.Factory registry consumer.TopicRegistry topicKey consumer.TopicKey consumerGroup string @@ -73,7 +73,7 @@ const opName = "process" func NewController( logger *zap.SugaredLogger, scope tally.Scope, - store storage.Storage, + stores storage.Factory, registry consumer.TopicRegistry, topicKey consumer.TopicKey, consumerGroup string, @@ -81,7 +81,7 @@ func NewController( return &Controller{ logger: logger.Named("speculate_controller"), metricsScope: scope.SubScope("speculate_controller"), - store: store, + stores: stores, registry: registry, topicKey: topicKey, consumerGroup: consumerGroup, @@ -99,7 +99,14 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er return fmt.Errorf("failed to deserialize batch ID: %w", err) } - batch, err := c.store.GetBatchStore().Get(ctx, bid.ID) + store, err := c.stores.For(storage.Config{QueueName: bid.Queue}) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "storage_resolve_errors", 1) + // Non-retryable: a missing or unresolvable queue is a malformed message. + return fmt.Errorf("failed to resolve storage for queue %q: %w", bid.Queue, err) + } + + batch, err := store.GetBatchStore().Get(ctx, bid.ID) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to get batch %s: %w", bid.ID, err) @@ -116,7 +123,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // speculate to drive to terminal. Cancel in-flight builds, fan out to // dependents, CAS to terminal Cancelled, and publish to conclude. if batch.State == entity.BatchStateCancelling { - return c.cancelBatch(ctx, batch) + return c.cancelBatch(ctx, store, batch) } // Terminal state: re-fan-out for self-healing in case a previous publish @@ -128,12 +135,12 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er metrics.NamedCounter(c.metricsScope, opName, "self_heal_terminal", 1) // Repair the membership record for the same crash window: a prior // attempt may have CAS'd to terminal without completing the record move. - if err := corebatch.EnsureRecord(ctx, c.store, batch); err != nil { + if err := corebatch.EnsureRecord(ctx, store, batch); err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return err } if batch.State == entity.BatchStateCancelled { - if err := c.respeculateDependents(ctx, batch); err != nil { + if err := c.respeculateDependents(ctx, store, batch); err != nil { return err } } @@ -145,7 +152,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er metrics.NamedCounter(c.metricsScope, opName, "noop_merging", 1) // A redelivery can land here after a tryFinalize attempt crashed // between its CAS and the record move; repair before acking. - if err := corebatch.EnsureRecord(ctx, c.store, batch); err != nil { + if err := corebatch.EnsureRecord(ctx, store, batch); err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return err } @@ -154,9 +161,9 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er switch batch.State { case entity.BatchStateCreated: - return c.startSpeculation(ctx, batch) + return c.startSpeculation(ctx, store, batch) case entity.BatchStateSpeculating: - return c.tryFinalize(ctx, batch) + return c.tryFinalize(ctx, store, batch) default: metrics.NamedCounter(c.metricsScope, opName, "unexpected_state", 1) return fmt.Errorf("unexpected batch state %q for batch %s", batch.State, batch.ID) @@ -165,7 +172,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // startSpeculation kicks off CI for this batch on top of the speculative head // (batch.Dependencies assumed to all pass), then transitions to Speculating. -func (c *Controller) startSpeculation(ctx context.Context, batch entity.Batch) error { +func (c *Controller) startSpeculation(ctx context.Context, store storage.Storage, batch entity.Batch) error { c.logger.Infow("starting speculation", "batch_id", batch.ID, "speculation_chain", append(append([]string{}, batch.Dependencies...), batch.ID), @@ -178,7 +185,7 @@ func (c *Controller) startSpeculation(ctx context.Context, batch entity.Batch) e // Optimistic CAS: if the version has already advanced (concurrent speculate), // the next event will see the new state and behave correctly. - if _, err := corebatch.Transition(ctx, c.store, batch, entity.BatchStateSpeculating); err != nil { + if _, err := corebatch.Transition(ctx, store, batch, entity.BatchStateSpeculating); err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return err } @@ -198,8 +205,8 @@ func (c *Controller) startSpeculation(ctx context.Context, batch entity.Batch) e // We will need to respeculate the failed paths — drop the failed dep // from the chain and re-issue speculation for the surviving ordering(s) // — instead of cascading the failure into requests that could still land. -func (c *Controller) tryFinalize(ctx context.Context, batch entity.Batch) error { - deps, err := c.fetchDependencies(ctx, batch) +func (c *Controller) tryFinalize(ctx context.Context, store storage.Storage, batch entity.Batch) error { + deps, err := c.fetchDependencies(ctx, store, batch) if err != nil { return err } @@ -218,7 +225,7 @@ func (c *Controller) tryFinalize(ctx context.Context, batch entity.Batch) error "dependency_id", d.ID, ) case entity.BatchStateFailed: - return c.failOnDependency(ctx, batch, d) + return c.failOnDependency(ctx, store, batch, d) default: pending = append(pending, d.ID) } @@ -238,7 +245,7 @@ func (c *Controller) tryFinalize(ctx context.Context, batch entity.Batch) error return fmt.Errorf("failed to publish to merge: %w", err) } - if _, err := corebatch.Transition(ctx, c.store, batch, entity.BatchStateMerging); err != nil { + if _, err := corebatch.Transition(ctx, store, batch, entity.BatchStateMerging); err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return err } @@ -251,7 +258,7 @@ func (c *Controller) tryFinalize(ctx context.Context, batch entity.Batch) error // the conclude queue so the request store and request log get reconciled. // Without this transition the batch would sit in Speculating forever — no // downstream event ever fires for it again. -func (c *Controller) failOnDependency(ctx context.Context, batch entity.Batch, dep entity.Batch) error { +func (c *Controller) failOnDependency(ctx context.Context, store storage.Storage, batch entity.Batch, dep entity.Batch) error { metrics.NamedCounter(c.metricsScope, opName, "dependency_failed", 1) c.logger.Warnw("dependency in non-succeeding terminal state; failing batch", "batch_id", batch.ID, @@ -259,7 +266,7 @@ func (c *Controller) failOnDependency(ctx context.Context, batch entity.Batch, d "dependency_state", string(dep.State), ) - batch, err := corebatch.Transition(ctx, c.store, batch, entity.BatchStateFailed) + batch, err := corebatch.Transition(ctx, store, batch, entity.BatchStateFailed) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return err @@ -305,7 +312,7 @@ func (c *Controller) failOnDependency(ctx context.Context, batch entity.Batch, d // storage.ErrVersionMismatch on the terminal CAS is returned as-is because it // is intrinsically retryable; the redelivery will land in the // self-heal branch and complete the fan-out. -func (c *Controller) cancelBatch(ctx context.Context, batch entity.Batch) error { +func (c *Controller) cancelBatch(ctx context.Context, store storage.Storage, batch entity.Batch) error { metrics.NamedCounter(c.metricsScope, opName, "cancel_batch", 1) c.logger.Infow("cancelling batch", "batch_id", batch.ID, @@ -317,17 +324,17 @@ func (c *Controller) cancelBatch(ctx context.Context, batch entity.Batch) error // collateral requests need a fresh request ID and a re-publish to TopicKeyStart so // they can be re-batched without the cancelled change. - if err := c.cancelBuild(ctx, batch); err != nil { + if err := c.cancelBuild(ctx, store, batch); err != nil { return err } - batch, err := corebatch.Transition(ctx, c.store, batch, entity.BatchStateCancelled) + batch, err := corebatch.Transition(ctx, store, batch, entity.BatchStateCancelled) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return err } - if err := c.respeculateDependents(ctx, batch); err != nil { + if err := c.respeculateDependents(ctx, store, batch); err != nil { return err } @@ -349,8 +356,8 @@ func (c *Controller) cancelBatch(ctx context.Context, batch entity.Batch) error // system has no external runner, so the local state flip is the complete // cancellation. Once a runner exists, it must be invoked here before the // local Update. -func (c *Controller) cancelBuild(ctx context.Context, batch entity.Batch) error { - build, err := c.store.GetBuildStore().Get(ctx, batch.ID) +func (c *Controller) cancelBuild(ctx context.Context, store storage.Storage, batch entity.Batch) error { + build, err := store.GetBuildStore().Get(ctx, batch.ID) if err != nil { if errors.Is(err, storage.ErrNotFound) { metrics.NamedCounter(c.metricsScope, opName, "cancel_build_not_found", 1) @@ -367,7 +374,7 @@ func (c *Controller) cancelBuild(ctx context.Context, batch entity.Batch) error updatedBuild := build updatedBuild.Status = entity.BuildStatusCancelled - if err := c.store.GetBuildStore().Update(ctx, updatedBuild); err != nil { + if err := store.GetBuildStore().Update(ctx, updatedBuild); err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to cancel build for batch %s: %w", batch.ID, err) } @@ -385,8 +392,8 @@ func (c *Controller) cancelBuild(ctx context.Context, batch entity.Batch) error // // Called both from the cancelBatch terminal flow and from the terminal // self-heal branch on redelivery of an already-Cancelled batch. -func (c *Controller) respeculateDependents(ctx context.Context, batch entity.Batch) error { - bd, err := c.store.GetBatchDependentStore().Get(ctx, batch.ID) +func (c *Controller) respeculateDependents(ctx context.Context, store storage.Storage, batch entity.Batch) error { + bd, err := store.GetBatchDependentStore().Get(ctx, batch.ID) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to get batch dependents for batch %s: %w", batch.ID, err) @@ -412,10 +419,10 @@ func (c *Controller) respeculateDependents(ctx context.Context, batch entity.Bat // is surfaced as a retryable infra failure; missing dependencies should not // happen in practice, but if one does it is treated the same as a transient // fetch failure (i.e. the message is retried). -func (c *Controller) fetchDependencies(ctx context.Context, batch entity.Batch) ([]entity.Batch, error) { +func (c *Controller) fetchDependencies(ctx context.Context, store storage.Storage, batch entity.Batch) ([]entity.Batch, error) { deps := make([]entity.Batch, 0, len(batch.Dependencies)) for _, depID := range batch.Dependencies { - d, err := c.store.GetBatchStore().Get(ctx, depID) + d, err := store.GetBatchStore().Get(ctx, depID) if err != nil { metrics.NamedCounter(c.metricsScope, opName, "dependency_fetch_errors", 1) return nil, fmt.Errorf("failed to get dependency batch %s of %s: %w", depID, batch.ID, err) diff --git a/submitqueue/orchestrator/controller/speculate/speculate_test.go b/submitqueue/orchestrator/controller/speculate/speculate_test.go index c5761a71..8961027e 100644 --- a/submitqueue/orchestrator/controller/speculate/speculate_test.go +++ b/submitqueue/orchestrator/controller/speculate/speculate_test.go @@ -37,10 +37,16 @@ import ( // newQueueBatchStateStore returns a QueueBatchStateStore mock that accepts any // membership-record write; these tests never list record buckets. +// staticStorageFactory resolves every queue to one fixed store aggregate. +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 } + func newQueueBatchStateStore(ctrl *gomock.Controller) *storagemock.MockQueueBatchStateStore { s := storagemock.NewMockQueueBatchStateStore(ctrl) s.EXPECT().Put(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - s.EXPECT().Delete(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + s.EXPECT().Delete(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() return s } @@ -94,7 +100,7 @@ func newTestController(t *testing.T, ctrl *gomock.Controller, store *storagemock ) require.NoError(t, err) - return NewController(logger, scope, store, registry, topickey.TopicKeySpeculate, "orchestrator-speculate") + return NewController(logger, scope, staticStorageFactory{store: store}, registry, topickey.TopicKeySpeculate, "orchestrator-speculate") } // runProcess builds a delivery for batchID and invokes Process once. @@ -301,7 +307,7 @@ func TestController_Process_TerminalSelfHeals(t *testing.T) { require.NoError(t, err) logger := zaptest.NewLogger(t).Sugar() - controller := NewController(logger, tally.NoopScope, store, registry, topickey.TopicKeySpeculate, "orchestrator-speculate") + controller := NewController(logger, tally.NoopScope, staticStorageFactory{store: store}, registry, topickey.TopicKeySpeculate, "orchestrator-speculate") require.NoError(t, runProcess(t, ctrl, controller, batch.ID)) }) @@ -357,7 +363,7 @@ func TestController_Process_CancelledTerminalSelfHealsDependents(t *testing.T) { require.NoError(t, err) logger := zaptest.NewLogger(t).Sugar() - controller := NewController(logger, tally.NoopScope, store, registry, topickey.TopicKeySpeculate, "orchestrator-speculate") + controller := NewController(logger, tally.NoopScope, staticStorageFactory{store: store}, registry, topickey.TopicKeySpeculate, "orchestrator-speculate") require.NoError(t, runProcess(t, ctrl, controller, batch.ID)) @@ -429,7 +435,7 @@ func TestController_Process_CancellingTerminalFlow(t *testing.T) { require.NoError(t, err) logger := zaptest.NewLogger(t).Sugar() - controller := NewController(logger, tally.NoopScope, store, registry, topickey.TopicKeySpeculate, "orchestrator-speculate") + controller := NewController(logger, tally.NoopScope, staticStorageFactory{store: store}, registry, topickey.TopicKeySpeculate, "orchestrator-speculate") require.NoError(t, runProcess(t, ctrl, controller, batch.ID)) @@ -541,7 +547,7 @@ func TestController_Process_CancellingNoDependents(t *testing.T) { require.NoError(t, err) logger := zaptest.NewLogger(t).Sugar() - controller := NewController(logger, tally.NoopScope, store, registry, topickey.TopicKeySpeculate, "orchestrator-speculate") + controller := NewController(logger, tally.NoopScope, staticStorageFactory{store: store}, registry, topickey.TopicKeySpeculate, "orchestrator-speculate") require.NoError(t, runProcess(t, ctrl, controller, batch.ID)) } @@ -583,7 +589,7 @@ func TestController_Process_CancellingTerminalCASVersionMismatch(t *testing.T) { require.NoError(t, err) logger := zaptest.NewLogger(t).Sugar() - controller := NewController(logger, tally.NoopScope, store, registry, topickey.TopicKeySpeculate, "orchestrator-speculate") + controller := NewController(logger, tally.NoopScope, staticStorageFactory{store: store}, registry, topickey.TopicKeySpeculate, "orchestrator-speculate") err = runProcess(t, ctrl, controller, batch.ID) require.Error(t, err) diff --git a/submitqueue/orchestrator/controller/start/start.go b/submitqueue/orchestrator/controller/start/start.go index b21d01e2..2b4c2ded 100644 --- a/submitqueue/orchestrator/controller/start/start.go +++ b/submitqueue/orchestrator/controller/start/start.go @@ -39,7 +39,7 @@ import ( type Controller struct { logger *zap.SugaredLogger metricsScope tally.Scope - store storage.Storage + stores storage.Factory registry consumer.TopicRegistry topicKey consumer.TopicKey consumerGroup string @@ -52,7 +52,7 @@ var _ consumer.Controller = (*Controller)(nil) func NewController( logger *zap.SugaredLogger, scope tally.Scope, - store storage.Storage, + stores storage.Factory, registry consumer.TopicRegistry, topicKey consumer.TopicKey, consumerGroup string, @@ -60,7 +60,7 @@ func NewController( return &Controller{ logger: logger.Named("start_controller"), metricsScope: scope.SubScope("start_controller"), - store: store, + stores: stores, registry: registry, topicKey: topicKey, consumerGroup: consumerGroup, @@ -82,6 +82,13 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er return fmt.Errorf("failed to deserialize land request: %w", err) } + store, err := c.stores.For(storage.Config{QueueName: landRequest.Queue}) + if err != nil { + metrics.NamedCounter(c.metricsScope, opName, "storage_resolve_errors", 1) + // Non-retryable: a missing or unresolvable queue is a malformed message. + return fmt.Errorf("failed to resolve storage for queue %q: %w", landRequest.Queue, err) + } + request := entity.Request{ ID: landRequest.ID, Queue: landRequest.Queue, @@ -105,7 +112,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // Persist request to storage. ErrAlreadyExists means a queue redelivery of the same // request_id (an at-least-once retry of THIS message), not a cross-request collision. - if err := c.store.GetRequestStore().Create(ctx, request); err != nil && !errors.Is(err, storage.ErrAlreadyExists) { + if err := store.GetRequestStore().Create(ctx, request); err != nil && !errors.Is(err, storage.ErrAlreadyExists) { metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1) return fmt.Errorf("failed to create request: %w", err) } diff --git a/submitqueue/orchestrator/controller/start/start_test.go b/submitqueue/orchestrator/controller/start/start_test.go index cea4d17a..12b88450 100644 --- a/submitqueue/orchestrator/controller/start/start_test.go +++ b/submitqueue/orchestrator/controller/start/start_test.go @@ -38,6 +38,12 @@ import ( ) // newTestController creates a controller with test dependencies. +// staticStorageFactory resolves every queue to one fixed store aggregate. +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 } + func newTestController( t *testing.T, ctrl *gomock.Controller, @@ -65,7 +71,7 @@ func newTestController( ) require.NoError(t, err) - return NewController(logger, scope, store, registry, topickey.TopicKeyStart, "orchestrator-start") + return NewController(logger, scope, staticStorageFactory{store: store}, registry, topickey.TopicKeyStart, "orchestrator-start") } // newMockStorage creates a MockStorage with a MockRequestStore that succeeds on Create. diff --git a/submitqueue/orchestrator/controller/validate/validate.go b/submitqueue/orchestrator/controller/validate/validate.go index 6d5496b0..e5362d32 100644 --- a/submitqueue/orchestrator/controller/validate/validate.go +++ b/submitqueue/orchestrator/controller/validate/validate.go @@ -44,7 +44,7 @@ import ( type Controller struct { logger *zap.SugaredLogger metricsScope tally.Scope - store storage.Storage + stores storage.Factory registry consumer.TopicRegistry changeProviders changeprovider.Factory validators validator.Factory @@ -63,7 +63,7 @@ var _ consumer.Controller = (*Controller)(nil) func NewController( logger *zap.SugaredLogger, scope tally.Scope, - store storage.Storage, + stores storage.Factory, registry consumer.TopicRegistry, changeProviders changeprovider.Factory, validators validator.Factory, @@ -74,7 +74,7 @@ func NewController( return &Controller{ logger: logger.Named("validate_controller"), metricsScope: scope.SubScope("validate_controller"), - store: store, + stores: stores, registry: registry, changeProviders: changeProviders, validators: validators, @@ -98,8 +98,15 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er return fmt.Errorf("failed to deserialize request ID: %w", err) } + store, err := c.stores.For(storage.Config{QueueName: rid.Queue}) + if err != nil { + coremetrics.NamedCounter(c.metricsScope, "process", "storage_resolve_errors", 1) + // Non-retryable: a missing or unresolvable queue is a malformed message. + return fmt.Errorf("failed to resolve storage for queue %q: %w", rid.Queue, err) + } + // Fetch request from storage - request, err := c.store.GetRequestStore().Get(ctx, rid.ID) + request, err := store.GetRequestStore().Get(ctx, rid.ID) if err != nil { coremetrics.NamedCounter(c.metricsScope, "process", "storage_errors", 1) return fmt.Errorf("failed to get request %s: %w", rid.ID, err) @@ -138,7 +145,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // claimed an overlapping URI in this queue. Per-queue partition leasing // (see platform/consumer + platform/extension/messagequeue) guarantees serial processing within // a queue, so the read-then-claim sequence below is race-free. - if dupID, err := c.checkDuplicate(ctx, request); err != nil { + if dupID, err := c.checkDuplicate(ctx, store, request); err != nil { return err } else if dupID != "" { c.logger.Infow("duplicate request detected", @@ -147,7 +154,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er "duplicate_id", dupID, ) coremetrics.NamedCounter(c.metricsScope, "process", "duplicate_requests", 1) - return c.reject(ctx, request.ID, fmt.Sprintf("request %s is a duplicate of in-flight request %s", request.ID, dupID)) + return c.reject(ctx, store, request.ID, fmt.Sprintf("request %s is a duplicate of in-flight request %s", request.ID, dupID)) } // Fetch change metadata @@ -186,7 +193,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er "queue", request.Queue, "error", err.Error(), ) - return c.reject(ctx, request.ID, fmt.Sprintf("custom validation failed: %v", err)) + return c.reject(ctx, store, request.ID, fmt.Sprintf("custom validation failed: %v", err)) } } } @@ -196,7 +203,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // rejected request never leaves a claim, and the record is written once with its // details (immutable thereafter; no separate enrichment update). Create is // idempotent per (queue, uri, request_id), so redelivery is a no-op. - if err := c.claimChanges(ctx, request, changeInfos); err != nil { + if err := c.claimChanges(ctx, store, request, changeInfos); err != nil { coremetrics.NamedCounter(c.metricsScope, "process", "change_store_errors", 1) return fmt.Errorf("failed to claim change records for request %s: %w", request.ID, err) } @@ -240,8 +247,8 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // Only an infra failure while terminating (storage/publish) is returned as an // error so the delivery is retried; the request itself is never re-queued for // validation once rejected. -func (c *Controller) reject(ctx context.Context, requestID, reason string) error { - if _, err := corerequest.TerminateRequest(ctx, c.store, c.registry, requestID, entity.RequestStateError, reason, nil); err != nil { +func (c *Controller) reject(ctx context.Context, store storage.Storage, requestID, reason string) error { + if _, err := corerequest.TerminateRequest(ctx, store, c.registry, requestID, entity.RequestStateError, reason, nil); err != nil { coremetrics.NamedCounter(c.metricsScope, "process", "terminate_errors", 1) return fmt.Errorf("failed to terminate rejected request %s: %w", requestID, err) } @@ -259,10 +266,10 @@ func (c *Controller) reject(ctx context.Context, requestID, reason string) error // // Per-URI / per-record reads keep the contract backend-agnostic; the typical request // has 1-5 URIs, so the loop is cheap. -func (c *Controller) checkDuplicate(ctx context.Context, request entity.Request) (string, error) { +func (c *Controller) checkDuplicate(ctx context.Context, store storage.Storage, request entity.Request) (string, error) { seenOwners := make(map[string]struct{}) for _, uri := range request.Change.URIs { - records, err := c.store.GetChangeStore().GetByURI(ctx, request.Queue, uri) + records, err := store.GetChangeStore().GetByURI(ctx, uri) if err != nil { coremetrics.NamedCounter(c.metricsScope, "process", "change_store_query_errors", 1) return "", fmt.Errorf("failed to query change store for request %s uri=%s: %w", request.ID, uri, err) @@ -276,7 +283,7 @@ func (c *Controller) checkDuplicate(ctx context.Context, request entity.Request) } seenOwners[rec.RequestID] = struct{}{} - owner, err := c.store.GetRequestStore().Get(ctx, rec.RequestID) + owner, err := store.GetRequestStore().Get(ctx, rec.RequestID) if errors.Is(err, storage.ErrNotFound) { continue } @@ -340,7 +347,7 @@ func toProtoStrategy(s mergestrategy.MergeStrategy) strategypb.Strategy { // and its Details are written together in a single immutable Create — there is no // later mutation. Create is idempotent on its primary key, so a redelivery (or a // prior partial attempt) is a no-op and the first write wins. -func (c *Controller) claimChanges(ctx context.Context, request entity.Request, infos []entity.ChangeInfo) error { +func (c *Controller) claimChanges(ctx context.Context, store storage.Storage, request entity.Request, infos []entity.ChangeInfo) error { now := time.Now().UnixMilli() for _, info := range infos { record := entity.ChangeRecord{ @@ -352,7 +359,7 @@ func (c *Controller) claimChanges(ctx context.Context, request entity.Request, i UpdatedAt: now, Version: 1, } - if err := c.store.GetChangeStore().Create(ctx, record); err != nil { + if err := store.GetChangeStore().Create(ctx, record); err != nil { return fmt.Errorf("failed to claim uri=%s for request %s: %w", info.URI, request.ID, err) } } diff --git a/submitqueue/orchestrator/controller/validate/validate_test.go b/submitqueue/orchestrator/controller/validate/validate_test.go index 2036e7f6..bff40c53 100644 --- a/submitqueue/orchestrator/controller/validate/validate_test.go +++ b/submitqueue/orchestrator/controller/validate/validate_test.go @@ -42,6 +42,12 @@ import ( "go.uber.org/zap/zaptest" ) +// staticStorageFactory resolves every queue to one fixed store aggregate. +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 } + func requestWithState(request entity.Request, state entity.RequestState) entity.Request { request.State = state return request @@ -90,7 +96,7 @@ func newMockStorage(ctrl *gomock.Controller, request entity.Request) (*storagemo // simulate overlap or assert the claim override these with their own EXPECTs. func newMockChangeStore(ctrl *gomock.Controller) *storagemock.MockChangeStore { cs := storagemock.NewMockChangeStore(ctrl) - cs.EXPECT().GetByURI(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() + cs.EXPECT().GetByURI(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes() cs.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() return cs } @@ -130,7 +136,7 @@ func newTestController( cpFactory := changeprovidermock.NewMockFactory(ctrl) cpFactory.EXPECT().For(gomock.Any()).Return(cp, nil).AnyTimes() - return NewController(logger, scope, store, registry, cpFactory, nil, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate") + return NewController(logger, scope, staticStorageFactory{store: store}, registry, cpFactory, nil, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate") } func TestNewController(t *testing.T) { @@ -212,7 +218,7 @@ func TestController_Process_PublishesCheckToRunway(t *testing.T) { cpFactory := changeprovidermock.NewMockFactory(ctrl) cpFactory.EXPECT().For(gomock.Any()).Return(&mockChangeProvider{}, nil).AnyTimes() - controller := NewController(logger, tally.NoopScope, store, registry, cpFactory, nil, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate") + controller := NewController(logger, tally.NoopScope, staticStorageFactory{store: store}, registry, cpFactory, nil, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate") msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) delivery := consumermock.NewMockDelivery(ctrl) @@ -259,7 +265,7 @@ func TestController_Process_ClaimsChangeRecordsWithDetails(t *testing.T) { } cs := storagemock.NewMockChangeStore(ctrl) // Duplicate-detection read finds no overlap. - cs.EXPECT().GetByURI(gomock.Any(), request.Queue, uri).Return(nil, nil).AnyTimes() + cs.EXPECT().GetByURI(gomock.Any(), uri).Return(nil, nil).AnyTimes() // Capture the record passed to Create; assert identity + details. cs.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn( func(_ context.Context, rec entity.ChangeRecord) error { @@ -486,7 +492,7 @@ func TestController_Process_DuplicateDetection(t *testing.T) { // One GetByURI per URI on the request, in order. Controller short-circuits on first // live duplicate, so .AnyTimes() lets unmatched URIs go un-queried. for _, u := range uris { - cs.EXPECT().GetByURI(gomock.Any(), queueName, u).Return(tt.byURI[u], nil).MaxTimes(1) + cs.EXPECT().GetByURI(gomock.Any(), u).Return(tt.byURI[u], nil).MaxTimes(1) } // When no duplicate is found, the controller continues to fetch change info // and claims each fetched change via Create. Accept any Create. @@ -530,7 +536,7 @@ func TestController_Process_ChangeStoreQueryFailure(t *testing.T) { store, _ := newMockStorage(ctrl, request) cs := storagemock.NewMockChangeStore(ctrl) - cs.EXPECT().GetByURI(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("change store down")) + cs.EXPECT().GetByURI(gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("change store down")) controller := newTestController(t, ctrl, store, cs, nil) @@ -619,7 +625,7 @@ func TestController_Process_CustomValidatorPasses(t *testing.T) { QueueName: request.Queue, }).Return(mockValidator, nil) - controller := NewController(logger, tally.NoopScope, store, registry, cpFactory, mockValidatorFactory, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate") + controller := NewController(logger, tally.NoopScope, staticStorageFactory{store: store}, registry, cpFactory, mockValidatorFactory, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate") msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) delivery := consumermock.NewMockDelivery(ctrl) @@ -678,7 +684,7 @@ func TestController_Process_CustomValidatorFails(t *testing.T) { QueueName: request.Queue, }).Return(mockValidator, nil) - controller := NewController(logger, tally.NoopScope, store, registry, cpFactory, mockValidatorFactory, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate") + controller := NewController(logger, tally.NoopScope, staticStorageFactory{store: store}, registry, cpFactory, mockValidatorFactory, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate") msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) delivery := consumermock.NewMockDelivery(ctrl) @@ -730,7 +736,7 @@ func TestController_Process_CustomValidatorFailure_TerminationPublishFails(t *te QueueName: request.Queue, }).Return(mockValidator, nil) - controller := NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, store, registry, cpFactory, mockValidatorFactory, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate") + controller := NewController(zaptest.NewLogger(t).Sugar(), tally.NoopScope, staticStorageFactory{store: store}, registry, cpFactory, mockValidatorFactory, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate") msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) delivery := consumermock.NewMockDelivery(ctrl) delivery.EXPECT().Message().Return(msg).AnyTimes() diff --git a/submitqueue/orchestrator/pipeline.go b/submitqueue/orchestrator/pipeline.go index 38b39033..1ba3c14c 100644 --- a/submitqueue/orchestrator/pipeline.go +++ b/submitqueue/orchestrator/pipeline.go @@ -56,8 +56,8 @@ type Deps struct { // Scope is the metrics scope for all controllers. Scope tally.Scope - // Storage provides request, batch, and change stores. - Storage storage.Storage + // Storage resolves the queue-scoped store aggregate per queue. + Storage storage.Factory // Counter provides distributed batch counters. Counter counter.Counter diff --git a/test/e2e/submitqueue/suite_test.go b/test/e2e/submitqueue/suite_test.go index 9dad3072..e1b40cf8 100644 --- a/test/e2e/submitqueue/suite_test.go +++ b/test/e2e/submitqueue/suite_test.go @@ -135,7 +135,9 @@ func (s *E2EIntegrationSuite) SetupSuite() { s.log.Logf("Schemas applied successfully") // White-box handle on the operating store for point-in-time RequestState. - s.requestStore = storagemysql.NewRequestStore(s.db, tally.NoopScope) + // Reads are not yet queue-filtered, so a single bound instance serves every + // e2e queue's point-in-time RequestState checks. + s.requestStore = storagemysql.NewRequestStore(s.db, tally.NoopScope, "e2e-test-queue") // Connect to Gateway gRPC service var gatewayConn *grpc.ClientConn diff --git a/test/integration/submitqueue/extension/storage/mysql/BUILD.bazel b/test/integration/submitqueue/extension/storage/mysql/BUILD.bazel index e14e602c..5e75fb89 100644 --- a/test/integration/submitqueue/extension/storage/mysql/BUILD.bazel +++ b/test/integration/submitqueue/extension/storage/mysql/BUILD.bazel @@ -12,6 +12,7 @@ go_test( "requires-network", ], deps = [ + "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mysql:go_default_library", "//test/integration/submitqueue/extension/storage:go_default_library", "//test/testutil:go_default_library", diff --git a/test/integration/submitqueue/extension/storage/mysql/storage_test.go b/test/integration/submitqueue/extension/storage/mysql/storage_test.go index 03e3ee4a..9bf58f3d 100644 --- a/test/integration/submitqueue/extension/storage/mysql/storage_test.go +++ b/test/integration/submitqueue/extension/storage/mysql/storage_test.go @@ -23,6 +23,7 @@ import ( "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "github.com/uber-go/tally" + "github.com/uber/submitqueue/submitqueue/extension/storage" mysqlstorage "github.com/uber/submitqueue/submitqueue/extension/storage/mysql" storagesuite "github.com/uber/submitqueue/test/integration/submitqueue/extension/storage" "github.com/uber/submitqueue/test/testutil" @@ -77,9 +78,11 @@ func (s *MySQLStorageIntegrationSuite) SetupSuite() { store, err := mysqlstorage.NewStorage(s.db, tally.NoopScope) require.NoError(t, err, "failed to create storage") - // Provide the storage instance to the contract suite + // Provide the storage backend to the contract suite: the queue-scoped + // factory adapter plus the global read-model stores. s.SetContext(ctx) - s.SetStorage(store) + s.SetFactory(mysqlFactory{backend: store}) + s.SetGlobalStores(store.GetRequestSummaryStore(), store.GetRequestURIStore()) s.SetLogger(s.log) t.Cleanup(func() { @@ -96,3 +99,14 @@ func (s *MySQLStorageIntegrationSuite) TearDownSuite() { s.log.Logf("Tearing down MySQL Storage integration test suite") // Cleanup handled automatically by testutil.ComposeStack } + +// mysqlFactory adapts the MySQL storage backend's queue binding to the +// storage.Factory seam for the contract suite, mirroring the host wiring. +type mysqlFactory struct { + backend *mysqlstorage.Storage +} + +// For returns the queue-scoped store aggregate bound to the queue named in config. +func (f mysqlFactory) For(config storage.Config) (storage.Storage, error) { + return f.backend.For(config.QueueName) +} diff --git a/test/integration/submitqueue/extension/storage/suite.go b/test/integration/submitqueue/extension/storage/suite.go index fa34de05..1d46afc9 100644 --- a/test/integration/submitqueue/extension/storage/suite.go +++ b/test/integration/submitqueue/extension/storage/suite.go @@ -30,14 +30,18 @@ import ( "github.com/uber/submitqueue/test/testutil" ) -// StorageContractSuite defines the contract tests for the storage.Storage interface. -// All storage implementations must pass these tests. -// Implementation-specific tests should embed this suite and call SetStorage(). +// 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(). type StorageContractSuite struct { suite.Suite - ctx context.Context - storage storage.Storage - log *testutil.TestLogger + ctx context.Context + factory storage.Factory + summaries storage.RequestSummaryStore + uris storage.RequestURIStore + log *testutil.TestLogger } // SetContext sets the context for tests @@ -45,9 +49,25 @@ func (s *StorageContractSuite) SetContext(ctx context.Context) { s.ctx = ctx } -// SetStorage is called by implementation tests to provide the concrete storage instance -func (s *StorageContractSuite) SetStorage(store storage.Storage) { - s.storage = store +// SetFactory is called by implementation tests to provide the queue-scoped +// storage factory under test. +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 { + store, err := s.factory.For(storage.Config{QueueName: queue}) + s.Require().NoError(err) + return store } // SetLogger sets the logger for tests @@ -72,11 +92,11 @@ func (s *StorageContractSuite) TestStorage_CreateAndGet() { } // Create request - err := s.storage.GetRequestStore().Create(ctx, request) + err := s.forQueue("test-queue").GetRequestStore().Create(ctx, request) require.NoError(t, err, "failed to create request") // Get request back - retrieved, err := s.storage.GetRequestStore().Get(ctx, request.ID) + retrieved, err := s.forQueue("test-queue").GetRequestStore().Get(ctx, request.ID) require.NoError(t, err, "failed to get request") // Verify fields @@ -113,11 +133,11 @@ func (s *StorageContractSuite) TestStorage_CreateAndGet_StackedPRs() { } // Create request - err := s.storage.GetRequestStore().Create(ctx, request) + err := s.forQueue("test-queue").GetRequestStore().Create(ctx, request) require.NoError(t, err, "failed to create request with stacked PRs") // Get request back - retrieved, err := s.storage.GetRequestStore().Get(ctx, request.ID) + retrieved, err := s.forQueue("test-queue").GetRequestStore().Get(ctx, request.ID) require.NoError(t, err, "failed to get request with stacked PRs") // Verify the stacked URIs are preserved @@ -141,19 +161,18 @@ func (s *StorageContractSuite) TestStorage_Update() { } // Create initial request - err := s.storage.GetRequestStore().Create(ctx, request) + err := s.forQueue("test-queue").GetRequestStore().Create(ctx, request) require.NoError(t, err) updated := request - updated.Queue = "updated-queue" updated.Change.URIs = []string{"github://github.example.com/uber/monorepo/pull/2/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"} updated.LandStrategy = mergestrategy.MergeStrategySquashRebase updated.State = entity.RequestStateProcessing - err = s.storage.GetRequestStore().Update(ctx, updated, request.Version, request.Version+1) + err = s.forQueue("test-queue").GetRequestStore().Update(ctx, updated, request.Version, request.Version+1) require.NoError(t, err, "failed to update request") // Verify update - retrieved, err := s.storage.GetRequestStore().Get(ctx, request.ID) + retrieved, err := s.forQueue("test-queue").GetRequestStore().Get(ctx, request.ID) require.NoError(t, err) updated.Version = request.Version + 1 assert.Equal(t, updated, retrieved) @@ -173,30 +192,28 @@ func (s *StorageContractSuite) TestStorage_OptimisticLocking() { } // Create request - err := s.storage.GetRequestStore().Create(ctx, request) + err := s.forQueue("test-queue").GetRequestStore().Create(ctx, request) require.NoError(t, err) // Update with correct version. updated := request - updated.Queue = "updated-queue" updated.Change.URIs = []string{"github://github.example.com/uber/monorepo/pull/2/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"} updated.LandStrategy = mergestrategy.MergeStrategySquashRebase updated.State = entity.RequestStateProcessing - err = s.storage.GetRequestStore().Update(ctx, updated, 1, 2) + err = s.forQueue("test-queue").GetRequestStore().Update(ctx, updated, 1, 2) require.NoError(t, err, "update with correct version should succeed") // Try to replace every field with a stale version. stale := request - stale.Queue = "stale-queue" stale.Change.URIs = []string{"github://github.example.com/uber/monorepo/pull/3/cccccccccccccccccccccccccccccccccccccccc"} stale.LandStrategy = mergestrategy.MergeStrategyRebase stale.State = entity.RequestStateLanded - err = s.storage.GetRequestStore().Update(ctx, stale, 1, 3) + err = s.forQueue("test-queue").GetRequestStore().Update(ctx, stale, 1, 3) assert.Error(t, err, "update with stale version should fail") assert.ErrorIs(t, err, storage.ErrVersionMismatch, "should return ErrVersionMismatch") // Verify no field was changed by the stale update. - retrieved, err := s.storage.GetRequestStore().Get(ctx, request.ID) + retrieved, err := s.forQueue("test-queue").GetRequestStore().Get(ctx, request.ID) require.NoError(t, err) updated.Version = 2 assert.Equal(t, updated, retrieved) @@ -225,13 +242,13 @@ func (s *StorageContractSuite) TestStorage_UpdateChangeURIs() { LandStrategy: mergestrategy.MergeStrategyMerge, Version: 1, } - require.NoError(t, s.storage.GetRequestStore().Create(ctx, request)) + require.NoError(t, s.forQueue("test-queue").GetRequestStore().Create(ctx, request)) updated := request updated.Change.URIs = tt.uris - require.NoError(t, s.storage.GetRequestStore().Update(ctx, updated, request.Version, request.Version+1)) + require.NoError(t, s.forQueue("test-queue").GetRequestStore().Update(ctx, updated, request.Version, request.Version+1)) - retrieved, err := s.storage.GetRequestStore().Get(ctx, request.ID) + retrieved, err := s.forQueue("test-queue").GetRequestStore().Get(ctx, request.ID) require.NoError(t, err) assert.Equal(t, tt.uris, retrieved.Change.URIs) }) @@ -248,22 +265,22 @@ func (s *StorageContractSuite) TestStorage_BatchDependentUpdate() { Dependents: []string{"test/dependent/1"}, Version: 1, } - require.NoError(t, s.storage.GetBatchDependentStore().Create(ctx, batchDependent)) + require.NoError(t, s.forQueue("test-queue").GetBatchDependentStore().Create(ctx, batchDependent)) nilDependents := batchDependent nilDependents.Dependents = nil - require.NoError(t, s.storage.GetBatchDependentStore().Update(ctx, nilDependents, 1, 2)) + require.NoError(t, s.forQueue("test-queue").GetBatchDependentStore().Update(ctx, nilDependents, 1, 2)) - retrieved, err := s.storage.GetBatchDependentStore().Get(ctx, batchDependent.BatchID) + retrieved, err := s.forQueue("test-queue").GetBatchDependentStore().Get(ctx, batchDependent.BatchID) require.NoError(t, err) assert.Nil(t, retrieved.Dependents) assert.Equal(t, int32(2), retrieved.Version) emptyDependents := retrieved emptyDependents.Dependents = []string{} - require.NoError(t, s.storage.GetBatchDependentStore().Update(ctx, emptyDependents, 2, 3)) + require.NoError(t, s.forQueue("test-queue").GetBatchDependentStore().Update(ctx, emptyDependents, 2, 3)) - retrieved, err = s.storage.GetBatchDependentStore().Get(ctx, batchDependent.BatchID) + retrieved, err = s.forQueue("test-queue").GetBatchDependentStore().Get(ctx, batchDependent.BatchID) require.NoError(t, err) assert.Empty(t, retrieved.Dependents) assert.NotNil(t, retrieved.Dependents) @@ -271,10 +288,10 @@ func (s *StorageContractSuite) TestStorage_BatchDependentUpdate() { staleUpdate := retrieved staleUpdate.Dependents = []string{"test/dependent/stale"} - err = s.storage.GetBatchDependentStore().Update(ctx, staleUpdate, 2, 4) + err = s.forQueue("test-queue").GetBatchDependentStore().Update(ctx, staleUpdate, 2, 4) assert.ErrorIs(t, err, storage.ErrVersionMismatch) - retrieved, err = s.storage.GetBatchDependentStore().Get(ctx, batchDependent.BatchID) + retrieved, err = s.forQueue("test-queue").GetBatchDependentStore().Get(ctx, batchDependent.BatchID) require.NoError(t, err) assert.Empty(t, retrieved.Dependents) assert.NotNil(t, retrieved.Dependents) @@ -284,7 +301,7 @@ func (s *StorageContractSuite) TestStorage_BatchDependentUpdate() { func (s *StorageContractSuite) TestStorage_BatchUpdateReplacesAllNonKeyFields() { t := s.T() ctx := s.ctx - store := s.storage.GetBatchStore() + store := s.forQueue("batch-update").GetBatchStore() batch := entity.Batch{ ID: "batch-update/batch/1", Queue: "batch-update", @@ -296,7 +313,6 @@ func (s *StorageContractSuite) TestStorage_BatchUpdateReplacesAllNonKeyFields() require.NoError(t, store.Create(ctx, batch)) nilCollections := batch - nilCollections.Queue = "batch-update-nil" nilCollections.Contains = nil nilCollections.Dependencies = nil nilCollections.State = entity.BatchStateSpeculating @@ -304,14 +320,13 @@ func (s *StorageContractSuite) TestStorage_BatchUpdateReplacesAllNonKeyFields() got, err := store.Get(ctx, batch.ID) require.NoError(t, err) - assert.Equal(t, "batch-update-nil", got.Queue) + assert.Equal(t, "batch-update", got.Queue) assert.Nil(t, got.Contains) assert.Nil(t, got.Dependencies) assert.Equal(t, entity.BatchStateSpeculating, got.State) assert.Equal(t, int32(2), got.Version) emptyCollections := got - emptyCollections.Queue = "batch-update-empty" emptyCollections.Contains = []string{} emptyCollections.Dependencies = []string{} emptyCollections.State = entity.BatchStateMerging @@ -319,7 +334,7 @@ func (s *StorageContractSuite) TestStorage_BatchUpdateReplacesAllNonKeyFields() got, err = store.Get(ctx, batch.ID) require.NoError(t, err) - assert.Equal(t, "batch-update-empty", got.Queue) + assert.Equal(t, "batch-update", got.Queue) assert.NotNil(t, got.Contains) assert.Empty(t, got.Contains) assert.NotNil(t, got.Dependencies) @@ -328,7 +343,6 @@ func (s *StorageContractSuite) TestStorage_BatchUpdateReplacesAllNonKeyFields() assert.Equal(t, int32(3), got.Version) stale := got - stale.Queue = "stale-queue" stale.Contains = []string{"stale/request"} stale.Dependencies = []string{"stale/batch"} stale.State = entity.BatchStateFailed @@ -345,53 +359,54 @@ func (s *StorageContractSuite) TestStorage_BatchUpdateReplacesAllNonKeyFields() func (s *StorageContractSuite) TestStorage_QueueBatchStateRecordLifecycle() { t := s.T() ctx := s.ctx - store := s.storage.GetQueueBatchStateStore() + storeA := s.forQueue("qbs-queue-a").GetQueueBatchStateStore() + storeB := s.forQueue("qbs-queue-b").GetQueueBatchStateStore() created1 := entity.QueueBatchState{Queue: "qbs-queue-a", State: entity.BatchStateCreated, BatchID: "qbs-queue-a/batch/1"} created2 := entity.QueueBatchState{Queue: "qbs-queue-a", State: entity.BatchStateCreated, BatchID: "qbs-queue-a/batch/2"} speculating := entity.QueueBatchState{Queue: "qbs-queue-a", State: entity.BatchStateSpeculating, BatchID: "qbs-queue-a/batch/3"} otherQueue := entity.QueueBatchState{Queue: "qbs-queue-b", State: entity.BatchStateCreated, BatchID: "qbs-queue-b/batch/1"} - require.NoError(t, store.Put(ctx, created1)) - require.NoError(t, store.Put(ctx, created2)) - require.NoError(t, store.Put(ctx, speculating)) - require.NoError(t, store.Put(ctx, otherQueue)) + require.NoError(t, storeA.Put(ctx, created1)) + require.NoError(t, storeA.Put(ctx, created2)) + require.NoError(t, storeA.Put(ctx, speculating)) + require.NoError(t, storeB.Put(ctx, otherQueue)) // Re-putting an existing record is a no-op success. - require.NoError(t, store.Put(ctx, created1)) + require.NoError(t, storeA.Put(ctx, created1)) // List returns exactly one (queue, state) bucket: no other states, no other queues, no duplicates. - got, err := store.List(ctx, "qbs-queue-a", entity.BatchStateCreated) + got, err := storeA.List(ctx, entity.BatchStateCreated) require.NoError(t, err) assert.ElementsMatch(t, []entity.QueueBatchState{created1, created2}, got) - got, err = store.List(ctx, "qbs-queue-a", entity.BatchStateSpeculating) + got, err = storeA.List(ctx, entity.BatchStateSpeculating) require.NoError(t, err) assert.ElementsMatch(t, []entity.QueueBatchState{speculating}, got) // An empty bucket lists empty, not an error. - got, err = store.List(ctx, "qbs-queue-a", entity.BatchStateMerging) + got, err = storeA.List(ctx, entity.BatchStateMerging) require.NoError(t, err) assert.Empty(t, got) // A record move: file under the new state, then remove the old bucket's record. moved := entity.QueueBatchState{Queue: created1.Queue, State: entity.BatchStateSpeculating, BatchID: created1.BatchID} - require.NoError(t, store.Put(ctx, moved)) - require.NoError(t, store.Delete(ctx, created1.Queue, created1.State, created1.BatchID)) + require.NoError(t, storeA.Put(ctx, moved)) + require.NoError(t, storeA.Delete(ctx, created1.State, created1.BatchID)) - got, err = store.List(ctx, "qbs-queue-a", entity.BatchStateCreated) + got, err = storeA.List(ctx, entity.BatchStateCreated) require.NoError(t, err) assert.ElementsMatch(t, []entity.QueueBatchState{created2}, got) - got, err = store.List(ctx, "qbs-queue-a", entity.BatchStateSpeculating) + got, err = storeA.List(ctx, entity.BatchStateSpeculating) require.NoError(t, err) assert.ElementsMatch(t, []entity.QueueBatchState{speculating, moved}, got) // Deleting an absent record is a no-op success. - require.NoError(t, store.Delete(ctx, created1.Queue, created1.State, created1.BatchID)) + require.NoError(t, storeA.Delete(ctx, created1.State, created1.BatchID)) // The other queue is untouched by all of the above. - got, err = store.List(ctx, "qbs-queue-b", entity.BatchStateCreated) + got, err = storeB.List(ctx, entity.BatchStateCreated) require.NoError(t, err) assert.ElementsMatch(t, []entity.QueueBatchState{otherQueue}, got) } @@ -402,7 +417,7 @@ func (s *StorageContractSuite) TestStorage_NotFound() { ctx := s.ctx // Try to get non-existent request - _, err := s.storage.GetRequestStore().Get(ctx, "test/nonexistent") + _, err := s.forQueue("test-queue").GetRequestStore().Get(ctx, "test/nonexistent") assert.Error(t, err, "getting non-existent request should return error") assert.ErrorIs(t, err, storage.ErrNotFound, "should return ErrNotFound") } @@ -421,11 +436,11 @@ func (s *StorageContractSuite) TestStorage_CreateDuplicate() { } // Create request - err := s.storage.GetRequestStore().Create(ctx, request) + err := s.forQueue("test-queue").GetRequestStore().Create(ctx, request) require.NoError(t, err) // Try to create duplicate - err = s.storage.GetRequestStore().Create(ctx, request) + err = s.forQueue("test-queue").GetRequestStore().Create(ctx, request) assert.Error(t, err, "creating duplicate request should return error") assert.ErrorIs(t, err, storage.ErrAlreadyExists, "should return ErrAlreadyExists") } @@ -442,11 +457,11 @@ func (s *StorageContractSuite) TestStorage_ChangeCreateAndGet_NoMatch() { ctx := s.ctx const queue = "cq-nomatch" - require.NoError(t, s.storage.GetChangeStore().Create(ctx, entity.ChangeRecord{ + require.NoError(t, s.forQueue(queue).GetChangeStore().Create(ctx, entity.ChangeRecord{ URI: changeURI, RequestID: queue + "/1", Queue: queue, CreatedAt: 1, UpdatedAt: 1, Version: 1, })) - got, err := s.storage.GetChangeStore().GetByURI(ctx, queue, "github://github.example.com/uber/x/pull/2/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb") + got, err := s.forQueue(queue).GetChangeStore().GetByURI(ctx, "github://github.example.com/uber/x/pull/2/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb") require.NoError(t, err) assert.Empty(t, got) } @@ -457,11 +472,11 @@ func (s *StorageContractSuite) TestStorage_ChangeCreateAndGet_Match() { ctx := s.ctx const queue = "cq-match" - require.NoError(t, s.storage.GetChangeStore().Create(ctx, entity.ChangeRecord{ + require.NoError(t, s.forQueue(queue).GetChangeStore().Create(ctx, entity.ChangeRecord{ URI: changeURI, RequestID: queue + "/1", Queue: queue, CreatedAt: 1, UpdatedAt: 1, Version: 1, })) - got, err := s.storage.GetChangeStore().GetByURI(ctx, queue, changeURI) + got, err := s.forQueue(queue).GetChangeStore().GetByURI(ctx, changeURI) require.NoError(t, err) require.Len(t, got, 1) assert.Equal(t, queue+"/1", got[0].RequestID) @@ -476,11 +491,11 @@ func (s *StorageContractSuite) TestStorage_ChangeGetByURI_DoesNotExcludeSelf() { ctx := s.ctx const queue = "cq-self" - require.NoError(t, s.storage.GetChangeStore().Create(ctx, entity.ChangeRecord{ + require.NoError(t, s.forQueue(queue).GetChangeStore().Create(ctx, entity.ChangeRecord{ URI: changeURI, RequestID: queue + "/1", Queue: queue, CreatedAt: 1, UpdatedAt: 1, Version: 1, })) - got, err := s.storage.GetChangeStore().GetByURI(ctx, queue, changeURI) + got, err := s.forQueue(queue).GetChangeStore().GetByURI(ctx, changeURI) require.NoError(t, err) require.Len(t, got, 1, "store returns the row even when caller might consider it self") assert.Equal(t, queue+"/1", got[0].RequestID) @@ -491,11 +506,11 @@ func (s *StorageContractSuite) TestStorage_ChangeGetByURI_QueueScoped() { t := s.T() ctx := s.ctx - require.NoError(t, s.storage.GetChangeStore().Create(ctx, entity.ChangeRecord{ + require.NoError(t, s.forQueue("cq-scoped-A").GetChangeStore().Create(ctx, entity.ChangeRecord{ URI: changeURI, RequestID: "cq-scoped-A/1", Queue: "cq-scoped-A", CreatedAt: 1, UpdatedAt: 1, Version: 1, })) - got, err := s.storage.GetChangeStore().GetByURI(ctx, "cq-scoped-B", changeURI) + got, err := s.forQueue("cq-scoped-B").GetChangeStore().GetByURI(ctx, changeURI) require.NoError(t, err) assert.Empty(t, got, "GetByURI must not return rows from a different queue") } @@ -507,10 +522,10 @@ func (s *StorageContractSuite) TestStorage_ChangeCreate_Idempotent() { const queue = "cq-idem" rec := entity.ChangeRecord{URI: changeURI, RequestID: queue + "/1", Queue: queue, CreatedAt: 1, UpdatedAt: 1, Version: 1} - require.NoError(t, s.storage.GetChangeStore().Create(ctx, rec)) - require.NoError(t, s.storage.GetChangeStore().Create(ctx, rec), "second insert with same PK must succeed (INSERT IGNORE)") + require.NoError(t, s.forQueue(queue).GetChangeStore().Create(ctx, rec)) + require.NoError(t, s.forQueue(queue).GetChangeStore().Create(ctx, rec), "second insert with same PK must succeed (INSERT IGNORE)") - got, err := s.storage.GetChangeStore().GetByURI(ctx, queue, changeURI) + got, err := s.forQueue(queue).GetChangeStore().GetByURI(ctx, changeURI) require.NoError(t, err) assert.Len(t, got, 1, "idempotent create must not duplicate rows") } @@ -521,14 +536,14 @@ func (s *StorageContractSuite) TestStorage_ChangeCreate_DifferentRequestSameURI( ctx := s.ctx const queue = "cq-multi" - require.NoError(t, s.storage.GetChangeStore().Create(ctx, entity.ChangeRecord{ + require.NoError(t, s.forQueue(queue).GetChangeStore().Create(ctx, entity.ChangeRecord{ URI: changeURI, RequestID: queue + "/1", Queue: queue, CreatedAt: 1, UpdatedAt: 1, Version: 1, })) - require.NoError(t, s.storage.GetChangeStore().Create(ctx, entity.ChangeRecord{ + require.NoError(t, s.forQueue(queue).GetChangeStore().Create(ctx, entity.ChangeRecord{ URI: changeURI, RequestID: queue + "/2", Queue: queue, CreatedAt: 2, UpdatedAt: 2, Version: 1, })) - got, err := s.storage.GetChangeStore().GetByURI(ctx, queue, changeURI) + got, err := s.forQueue(queue).GetChangeStore().GetByURI(ctx, changeURI) require.NoError(t, err) require.Len(t, got, 2) @@ -555,11 +570,11 @@ func (s *StorageContractSuite) TestStorage_ChangeCreate_PreservesDetails() { const queue = "cq-details" details := sampleDetails() - require.NoError(t, s.storage.GetChangeStore().Create(ctx, entity.ChangeRecord{ + require.NoError(t, s.forQueue(queue).GetChangeStore().Create(ctx, entity.ChangeRecord{ URI: changeURI, RequestID: queue + "/1", Queue: queue, Details: details, CreatedAt: 1, UpdatedAt: 1, Version: 1, })) - got, err := s.storage.GetChangeStore().GetByURI(ctx, queue, changeURI) + got, err := s.forQueue(queue).GetChangeStore().GetByURI(ctx, changeURI) require.NoError(t, err) require.Len(t, got, 1) assert.Equal(t, details, got[0].Details) @@ -571,11 +586,11 @@ func (s *StorageContractSuite) TestStorage_ChangeCreate_EmptyDetails() { ctx := s.ctx const queue = "cq-emptydetails" - require.NoError(t, s.storage.GetChangeStore().Create(ctx, entity.ChangeRecord{ + require.NoError(t, s.forQueue(queue).GetChangeStore().Create(ctx, entity.ChangeRecord{ URI: changeURI, RequestID: queue + "/1", Queue: queue, CreatedAt: 1, UpdatedAt: 1, Version: 1, })) - got, err := s.storage.GetChangeStore().GetByURI(ctx, queue, changeURI) + got, err := s.forQueue(queue).GetChangeStore().GetByURI(ctx, changeURI) require.NoError(t, err) require.Len(t, got, 1) assert.Equal(t, entity.ChangeDetails{}, got[0].Details) @@ -596,9 +611,9 @@ func (s *StorageContractSuite) TestStorage_BuildCreateAndGet() { Status: entity.BuildStatusRunning, } - require.NoError(t, s.storage.GetBuildStore().Create(ctx, build)) + require.NoError(t, s.forQueue("test-queue").GetBuildStore().Create(ctx, build)) - got, err := s.storage.GetBuildStore().Get(ctx, buildID) + got, err := s.forQueue("test-queue").GetBuildStore().Get(ctx, buildID) require.NoError(t, err, "Get by ID should find the build") assert.Equal(t, build.ID, got.ID) assert.Equal(t, build.BatchID, got.BatchID) @@ -613,7 +628,7 @@ func (s *StorageContractSuite) TestStorage_RequestSummaryCreateGetAndCAS() { Status: entity.RequestStatusAccepted, RequestVersion: 1, StatusTimestampMs: 100, Version: 1, LastError: "", Metadata: nil, } - store := s.storage.GetRequestSummaryStore() + store := s.summaries require.NoError(t, store.Create(ctx, summary)) require.ErrorIs(t, store.Create(ctx, summary), storage.ErrAlreadyExists) @@ -689,7 +704,7 @@ func (s *StorageContractSuite) TestStorage_RequestSummaryCreateGetAndCAS() { func (s *StorageContractSuite) TestStorage_RequestQueueSummaryListAndCursor() { t := s.T() ctx := s.ctx - store := s.storage.GetRequestQueueSummaryStore() + store := s.forQueue("queue-summary").GetRequestQueueSummaryStore() rows := []entity.RequestQueueSummary{ {RequestID: "queue-summary/1", Queue: "queue-summary", ChangeURIs: nil, ReceivedAtMs: 100, Status: entity.RequestStatusAccepted, Version: 1, Metadata: nil}, {RequestID: "queue-summary/2", Queue: "queue-summary", ChangeURIs: []string{"uri/2"}, ReceivedAtMs: 200, Status: entity.RequestStatusLanded, Version: 1, Metadata: map[string]string{}}, @@ -700,11 +715,11 @@ func (s *StorageContractSuite) TestStorage_RequestQueueSummaryListAndCursor() { } require.ErrorIs(t, store.Create(ctx, rows[0]), storage.ErrAlreadyExists) - got, err := store.Get(ctx, rows[0].Queue, rows[0].ReceivedAtMs, rows[0].RequestID) + got, err := store.Get(ctx, rows[0].ReceivedAtMs, rows[0].RequestID) require.NoError(t, err) assert.NotNil(t, got.ChangeURIs) assert.NotNil(t, got.Metadata) - _, err = store.Get(ctx, "queue-summary", 999, "queue-summary/missing") + _, err = store.Get(ctx, 999, "queue-summary/missing") require.ErrorIs(t, err, storage.ErrNotFound) got.Status = entity.RequestStatusLanded @@ -712,7 +727,7 @@ func (s *StorageContractSuite) TestStorage_RequestQueueSummaryListAndCursor() { got.LastError = "done" got.Metadata = map[string]string{"result": "landed"} require.NoError(t, store.Update(ctx, got, 1, 2)) - updated, err := store.Get(ctx, got.Queue, got.ReceivedAtMs, got.RequestID) + updated, err := store.Get(ctx, got.ReceivedAtMs, got.RequestID) require.NoError(t, err) assert.Equal(t, int32(2), updated.Version) assert.Equal(t, []string{"uri/replacement/1", "uri/replacement/2"}, updated.ChangeURIs) @@ -726,14 +741,14 @@ func (s *StorageContractSuite) TestStorage_RequestQueueSummaryListAndCursor() { stale.LastError = "stale" stale.Metadata = map[string]string{} require.ErrorIs(t, store.Update(ctx, stale, 1, 3), storage.ErrVersionMismatch) - unchanged, err := store.Get(ctx, got.Queue, got.ReceivedAtMs, got.RequestID) + unchanged, err := store.Get(ctx, got.ReceivedAtMs, got.RequestID) require.NoError(t, err) assert.Equal(t, updated, unchanged) updated.ChangeURIs = nil updated.Metadata = nil require.NoError(t, store.Update(ctx, updated, 2, 3)) - normalized, err := store.Get(ctx, got.Queue, got.ReceivedAtMs, got.RequestID) + normalized, err := store.Get(ctx, got.ReceivedAtMs, got.RequestID) require.NoError(t, err) assert.Equal(t, int32(3), normalized.Version) assert.NotNil(t, normalized.ChangeURIs) @@ -744,7 +759,7 @@ func (s *StorageContractSuite) TestStorage_RequestQueueSummaryListAndCursor() { normalized.ChangeURIs = []string{} normalized.Metadata = map[string]string{} require.NoError(t, store.Update(ctx, normalized, 3, 4)) - emptyCollections, err := store.Get(ctx, got.Queue, got.ReceivedAtMs, got.RequestID) + emptyCollections, err := store.Get(ctx, got.ReceivedAtMs, got.RequestID) require.NoError(t, err) assert.Equal(t, int32(4), emptyCollections.Version) assert.NotNil(t, emptyCollections.ChangeURIs) @@ -753,14 +768,14 @@ func (s *StorageContractSuite) TestStorage_RequestQueueSummaryListAndCursor() { assert.Empty(t, emptyCollections.Metadata) firstPage, err := store.List(ctx, storage.RequestQueueSummaryQuery{ - Queue: "queue-summary", ReceivedAtOrAfterMs: 50, ReceivedBeforeMs: 250, Limit: 2, + ReceivedAtOrAfterMs: 50, ReceivedBeforeMs: 250, Limit: 2, }) require.NoError(t, err) require.Len(t, firstPage, 2) assert.Equal(t, []string{"queue-summary/3", "queue-summary/2"}, []string{firstPage[0].RequestID, firstPage[1].RequestID}) secondPage, err := store.List(ctx, storage.RequestQueueSummaryQuery{ - Queue: "queue-summary", ReceivedAtOrAfterMs: 50, ReceivedBeforeMs: 250, Limit: 2, + ReceivedAtOrAfterMs: 50, ReceivedBeforeMs: 250, Limit: 2, HasCursor: true, Cursor: storage.RequestQueueSummaryCursor{ReceivedAtMs: 200, RequestID: "queue-summary/2"}, }) require.NoError(t, err) @@ -770,14 +785,14 @@ func (s *StorageContractSuite) TestStorage_RequestQueueSummaryListAndCursor() { assert.NotNil(t, secondPage[0].Metadata) bounded, err := store.List(ctx, storage.RequestQueueSummaryQuery{ - Queue: "queue-summary", ReceivedAtOrAfterMs: 100, ReceivedBeforeMs: 200, Limit: 10, + ReceivedAtOrAfterMs: 100, ReceivedBeforeMs: 200, Limit: 10, }) require.NoError(t, err) require.Len(t, bounded, 1) assert.Equal(t, "queue-summary/1", bounded[0].RequestID) empty, err := store.List(ctx, storage.RequestQueueSummaryQuery{ - Queue: "queue-summary", ReceivedAtOrAfterMs: 300, ReceivedBeforeMs: 400, Limit: 10, + ReceivedAtOrAfterMs: 300, ReceivedBeforeMs: 400, Limit: 10, }) require.NoError(t, err) assert.Empty(t, empty) @@ -786,7 +801,7 @@ func (s *StorageContractSuite) TestStorage_RequestQueueSummaryListAndCursor() { func (s *StorageContractSuite) TestStorage_RequestURIListIsBoundedAndOrdered() { t := s.T() ctx := s.ctx - store := s.storage.GetRequestURIStore() + store := s.uris rows := []entity.RequestURI{ {ChangeURI: "uri/shared", ReceivedAtMs: 100, RequestID: "uri/1"}, {ChangeURI: "uri/shared", ReceivedAtMs: 200, RequestID: "uri/2"}, diff --git a/test/integration/submitqueue/gateway/BUILD.bazel b/test/integration/submitqueue/gateway/BUILD.bazel index 19476900..ec8bc96b 100644 --- a/test/integration/submitqueue/gateway/BUILD.bazel +++ b/test/integration/submitqueue/gateway/BUILD.bazel @@ -23,6 +23,7 @@ go_test( "//submitqueue/core/request:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", + "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mysql:go_default_library", "//test/testutil:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", diff --git a/test/integration/submitqueue/gateway/suite_test.go b/test/integration/submitqueue/gateway/suite_test.go index a49e39c1..c7b3528a 100644 --- a/test/integration/submitqueue/gateway/suite_test.go +++ b/test/integration/submitqueue/gateway/suite_test.go @@ -43,6 +43,7 @@ import ( corerequest "github.com/uber/submitqueue/submitqueue/core/request" "github.com/uber/submitqueue/submitqueue/core/topickey" "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/storage" mysqlstorage "github.com/uber/submitqueue/submitqueue/extension/storage/mysql" "github.com/uber/submitqueue/test/testutil" "go.uber.org/zap" @@ -171,7 +172,7 @@ func (s *GatewayIntegrationSuite) TestListAPI() { t := s.T() store, err := mysqlstorage.NewStorage(s.db, tally.NoopScope) require.NoError(t, err) - materializer := corerequest.NewMaterializer(store) + materializer := corerequest.NewMaterializer(store.GetRequestLogStore(), store.GetRequestSummaryStore(), store.GetRequestURIStore(), mysqlFactory{backend: store}) 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{}}, @@ -296,3 +297,14 @@ func (s *GatewayIntegrationSuite) TestRequestLogConsumer() { s.log.Logf("Request log consumer test passed: entry persisted and readable via GetRequestSummaryByID") } + +// mysqlFactory adapts the MySQL storage backend's queue binding to the +// storage.Factory seam, mirroring the host wiring. +type mysqlFactory struct { + backend *mysqlstorage.Storage +} + +// For returns the queue-scoped store aggregate bound to the queue named in config. +func (f mysqlFactory) For(config storage.Config) (storage.Storage, error) { + return f.backend.For(config.QueueName) +}