Skip to content

Commit e280a79

Browse files
committed
feat(submitqueue): activate request status materialization
Route synchronous, queued, and DLQ request-log writes through the shared materializer so authoritative and queue projections converge with retained history. Preserve the original DLQ failure reason as the terminal request last error. Validation: make fmt && make build && make test && make e2e-test
1 parent 1fe902a commit e280a79

24 files changed

Lines changed: 354 additions & 201 deletions

File tree

service/submitqueue/gateway/server/main.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,7 @@ func run() error {
271271
// Create controllers and wrap them for gRPC
272272
pingController := controller.NewPingController(logger, scope)
273273
landController := controller.NewLandController(logger.Sugar(), scope, cnt, store, queueConfigs, registry)
274-
cancelController := controller.NewCancelController(logger.Sugar(), scope, requestLogStore, registry)
274+
cancelController := controller.NewCancelController(logger.Sugar(), scope, store, registry)
275275
statusController := controller.NewStatusController(logger.Sugar(), scope, requestLogStore)
276276
gatewayServer := &GatewayServer{
277277
pingController: pingController,

submitqueue/gateway/controller/cancel.go

Lines changed: 17 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
entityqueue "github.com/uber/submitqueue/platform/base/messagequeue"
2424
"github.com/uber/submitqueue/platform/consumer"
2525
"github.com/uber/submitqueue/platform/errs"
26+
requestcore "github.com/uber/submitqueue/submitqueue/core/request"
2627
"github.com/uber/submitqueue/submitqueue/core/topickey"
2728
"github.com/uber/submitqueue/submitqueue/entity"
2829
"github.com/uber/submitqueue/submitqueue/extension/storage"
@@ -35,21 +36,23 @@ import (
3536
// and returns a response. The orchestrator-side cancel controller performs the actual
3637
// state transitions and emits the terminal RequestStatusCancelled log entry.
3738
type CancelController struct {
38-
logger *zap.SugaredLogger
39-
metricsScope tally.Scope
40-
requestLogStore storage.RequestLogStore
41-
registry consumer.TopicRegistry
39+
logger *zap.SugaredLogger
40+
metricsScope tally.Scope
41+
requestSummaryStore storage.RequestSummaryStore
42+
materializer *requestcore.Materializer
43+
registry consumer.TopicRegistry
4244
}
4345

4446
// NewCancelController creates a new instance of the gateway cancel controller.
45-
// The controller writes a RequestStatusCancelling log entry through requestLogStore and
47+
// The controller writes a RequestStatusCancelling log entry through the shared materializer and
4648
// publishes cancel requests to the topic registered under topickey.TopicKeyCancel.
47-
func NewCancelController(logger *zap.SugaredLogger, scope tally.Scope, requestLogStore storage.RequestLogStore, registry consumer.TopicRegistry) *CancelController {
49+
func NewCancelController(logger *zap.SugaredLogger, scope tally.Scope, store storage.Storage, registry consumer.TopicRegistry) *CancelController {
4850
return &CancelController{
49-
logger: logger,
50-
metricsScope: scope,
51-
requestLogStore: requestLogStore,
52-
registry: registry,
51+
logger: logger,
52+
metricsScope: scope,
53+
requestSummaryStore: store.GetRequestSummaryStore(),
54+
materializer: requestcore.NewMaterializer(store),
55+
registry: registry,
5356
}
5457
}
5558

@@ -78,18 +81,13 @@ func (c *CancelController) Cancel(ctx context.Context, req entity.CancelRequest)
7881
"reason", req.Reason,
7982
)
8083

81-
// Verify the sqid exists before recording intent or publishing. Cancel is opt-in
82-
// by sqid; an unknown sqid is a user error and must never leave a cancelling log
83-
// row or a queue message behind for a request that never existed. The Land
84-
// controller writes its "accepted" log entry synchronously to the same store, so
85-
// a NotFound here reliably means "this sqid was never accepted by the gateway"
86-
// rather than "in flight" — there is no false-negative race window.
87-
if _, err := c.requestLogStore.List(ctx, req.ID); err != nil {
84+
// Verify the sqid exists before recording intent or publishing.
85+
if _, err := c.requestSummaryStore.Get(ctx, req.ID); err != nil {
8886
if storage.IsNotFound(err) {
8987
c.metricsScope.Counter("cancel_request_not_found").Inc(1)
9088
return errs.NewUserError(&RequestNotFoundError{Sqid: req.ID})
9189
}
92-
return fmt.Errorf("CancelController failed to look up request log for sqid=%s: %w", req.ID, err)
90+
return fmt.Errorf("CancelController failed to look up request summary for sqid=%s: %w", req.ID, err)
9391
}
9492

9593
// Record the user's intent in the request log before publishing. Writing direct to the
@@ -100,7 +98,7 @@ func (c *CancelController) Cancel(ctx context.Context, req entity.CancelRequest)
10098
metadata["reason"] = req.Reason
10199
}
102100
logEntry := entity.NewRequestLog(req.ID, entity.RequestStatusCancelling, 0, "", metadata)
103-
if err := c.requestLogStore.Insert(ctx, logEntry); err != nil {
101+
if err := c.materializer.PersistLog(ctx, logEntry); err != nil {
104102
return fmt.Errorf("CancelController failed to insert cancelling log for sqid=%s: %w", req.ID, err)
105103
}
106104

submitqueue/gateway/controller/cancel_test.go

Lines changed: 36 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ import (
2828
queuemock "github.com/uber/submitqueue/platform/extension/messagequeue/mock"
2929
"github.com/uber/submitqueue/submitqueue/core/topickey"
3030
"github.com/uber/submitqueue/submitqueue/entity"
31-
"github.com/uber/submitqueue/submitqueue/extension/storage"
3231
storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock"
3332
"go.uber.org/mock/gomock"
3433
"go.uber.org/zap"
@@ -57,14 +56,16 @@ func newCancelTestRegistryWithNoopPublisher(t *testing.T, ctrl *gomock.Controlle
5756
return registry
5857
}
5958

60-
// newRequestLogStoreNoop returns a RequestLogStore mock whose List returns a single
61-
// dummy entry (so existence check passes) and whose Insert silently succeeds for any input.
62-
func newRequestLogStoreNoop(t *testing.T, ctrl *gomock.Controller) *storagemock.MockRequestLogStore {
63-
t.Helper()
64-
store := storagemock.NewMockRequestLogStore(ctrl)
65-
store.EXPECT().List(gomock.Any(), gomock.Any()).Return([]entity.RequestLog{{}}, nil).AnyTimes()
66-
store.EXPECT().Insert(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
67-
return store
59+
// newCancelStorageFixture returns a storage fixture with one received request.
60+
func newCancelStorageFixture(ctrl *gomock.Controller, requestID string) *controllerStorageFixture {
61+
fixture := newControllerStorageFixture(ctrl)
62+
if requestID != "" {
63+
fixture.addSummary(entity.RequestSummary{
64+
RequestID: requestID, Queue: "test-queue", ChangeURIs: []string{}, ReceivedAtMs: 1,
65+
Status: entity.RequestStatusAccepted, StatusTimestampMs: 1, Version: 1, Metadata: map[string]string{},
66+
})
67+
}
68+
return fixture
6869
}
6970

7071
// testCancelRequest returns a valid entity.CancelRequest for testing.
@@ -75,14 +76,14 @@ func testCancelRequest(sqid string, reason string) entity.CancelRequest {
7576
func TestNewCancelController(t *testing.T) {
7677
ctrl := gomock.NewController(t)
7778

78-
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newRequestLogStoreNoop(t, ctrl), newCancelTestRegistryWithNoopPublisher(t, ctrl))
79+
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newCancelStorageFixture(ctrl, "test-queue/42").storage, newCancelTestRegistryWithNoopPublisher(t, ctrl))
7980
require.NotNil(t, controller)
8081
}
8182

8283
func TestCancel_HappyPath(t *testing.T) {
8384
ctrl := gomock.NewController(t)
8485

85-
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newRequestLogStoreNoop(t, ctrl), newCancelTestRegistryWithNoopPublisher(t, ctrl))
86+
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newCancelStorageFixture(ctrl, "test-queue/42").storage, newCancelTestRegistryWithNoopPublisher(t, ctrl))
8687
ctx := context.Background()
8788

8889
err := controller.Cancel(ctx, testCancelRequest("test-queue/42", "user changed their mind"))
@@ -93,7 +94,7 @@ func TestCancel_HappyPath(t *testing.T) {
9394
func TestCancel_ReturnsErrorOnEmptySqid(t *testing.T) {
9495
ctrl := gomock.NewController(t)
9596

96-
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newRequestLogStoreNoop(t, ctrl), newCancelTestRegistryWithNoopPublisher(t, ctrl))
97+
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newCancelStorageFixture(ctrl, "test-queue/42").storage, newCancelTestRegistryWithNoopPublisher(t, ctrl))
9798
ctx := context.Background()
9899

99100
err := controller.Cancel(ctx, testCancelRequest("", "anything"))
@@ -117,7 +118,7 @@ func TestCancel_PublishesToQueue(t *testing.T) {
117118
},
118119
)
119120

120-
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newRequestLogStoreNoop(t, ctrl), registry)
121+
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newCancelStorageFixture(ctrl, "my-queue/7").storage, registry)
121122
ctx := context.Background()
122123

123124
err := controller.Cancel(ctx, testCancelRequest("my-queue/7", "obsolete change"))
@@ -138,31 +139,28 @@ func TestCancel_PublishesToQueue(t *testing.T) {
138139
// before the cancel topic publish so observers see intent the moment Cancel returns.
139140
func TestCancel_InsertsCancellingLog(t *testing.T) {
140141
ctrl := gomock.NewController(t)
141-
142-
var insertedLog entity.RequestLog
143-
logStore := storagemock.NewMockRequestLogStore(ctrl)
144-
logStore.EXPECT().List(gomock.Any(), "my-queue/42").Return([]entity.RequestLog{{}}, nil)
145-
logStore.EXPECT().Insert(gomock.Any(), gomock.Any()).DoAndReturn(
146-
func(_ context.Context, entry entity.RequestLog) error {
147-
insertedLog = entry
148-
return nil
149-
},
150-
).Times(1)
142+
fixture := newCancelStorageFixture(ctrl, "my-queue/42")
151143

152144
registry, publisher := newCancelTestRegistry(t, ctrl)
153145
insertedBeforePublish := false
154146
publisher.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(
155147
func(_ context.Context, _ string, _ entityqueue.Message) error {
156-
insertedBeforePublish = insertedLog.RequestID != ""
148+
fixture.mu.Lock()
149+
defer fixture.mu.Unlock()
150+
insertedBeforePublish = len(fixture.logs) == 1
157151
return nil
158152
},
159153
)
160154

161-
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, logStore, registry)
155+
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, fixture.storage, registry)
162156

163157
err := controller.Cancel(context.Background(), testCancelRequest("my-queue/42", "obsolete change"))
164158
require.NoError(t, err)
165159

160+
fixture.mu.Lock()
161+
require.Len(t, fixture.logs, 1)
162+
insertedLog := fixture.logs[0]
163+
fixture.mu.Unlock()
166164
assert.Equal(t, "my-queue/42", insertedLog.RequestID)
167165
assert.Equal(t, entity.RequestStatusCancelling, insertedLog.Status)
168166
assert.Equal(t, "obsolete change", insertedLog.Metadata["reason"])
@@ -173,15 +171,13 @@ func TestCancel_InsertsCancellingLog(t *testing.T) {
173171
// short-circuits the RPC with an error and the cancel topic is never published to.
174172
func TestCancel_LogInsertFailure(t *testing.T) {
175173
ctrl := gomock.NewController(t)
176-
177-
logStore := storagemock.NewMockRequestLogStore(ctrl)
178-
logStore.EXPECT().List(gomock.Any(), "q/1").Return([]entity.RequestLog{{}}, nil)
179-
logStore.EXPECT().Insert(gomock.Any(), gomock.Any()).Return(fmt.Errorf("db unavailable"))
174+
fixture := newCancelStorageFixture(ctrl, "q/1")
175+
fixture.setLogInsertError(fmt.Errorf("db unavailable"))
180176

181177
registry, publisher := newCancelTestRegistry(t, ctrl)
182178
_ = publisher
183179

184-
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, logStore, registry)
180+
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, fixture.storage, registry)
185181
err := controller.Cancel(context.Background(), testCancelRequest("q/1", ""))
186182
require.Error(t, err)
187183
}
@@ -192,27 +188,22 @@ func TestCancel_ReturnsErrorOnPublishFailure(t *testing.T) {
192188
registry, publisher := newCancelTestRegistry(t, ctrl)
193189
publisher.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("queue unavailable"))
194190

195-
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newRequestLogStoreNoop(t, ctrl), registry)
191+
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newCancelStorageFixture(ctrl, "test-queue/1").storage, registry)
196192
ctx := context.Background()
197193

198194
err := controller.Cancel(ctx, testCancelRequest("test-queue/1", ""))
199195

200196
require.Error(t, err)
201197
}
202198

203-
// TestCancel_UnknownSqidIsUserError asserts that Cancel for a sqid with no
204-
// request_log history fails fast with a RequestNotFoundError (user error) and
205-
// never inserts a cancelling log row or publishes to the cancel topic.
206199
func TestCancel_UnknownSqidIsUserError(t *testing.T) {
207200
ctrl := gomock.NewController(t)
208-
209-
logStore := storagemock.NewMockRequestLogStore(ctrl)
210-
logStore.EXPECT().List(gomock.Any(), "ghost/1").Return(nil, storage.ErrNotFound)
201+
fixture := newCancelStorageFixture(ctrl, "")
211202

212203
registry, publisher := newCancelTestRegistry(t, ctrl)
213204
_ = publisher
214205

215-
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, logStore, registry)
206+
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, fixture.storage, registry)
216207
err := controller.Cancel(context.Background(), testCancelRequest("ghost/1", ""))
217208
require.Error(t, err)
218209
assert.True(t, IsRequestNotFound(err))
@@ -224,19 +215,21 @@ func TestCancel_UnknownSqidIsUserError(t *testing.T) {
224215
assert.Equal(t, "ghost/1", typed.Sqid)
225216
}
226217

227-
// TestCancel_RequestLogLookupFailure asserts that an infrastructure failure on
218+
// TestCancel_RequestSummaryLookupFailure asserts that an infrastructure failure on
228219
// the existence check propagates as a (non-user) error and skips the rest of
229220
// the pipeline.
230-
func TestCancel_RequestLogLookupFailure(t *testing.T) {
221+
func TestCancel_RequestSummaryLookupFailure(t *testing.T) {
231222
ctrl := gomock.NewController(t)
232223

233-
logStore := storagemock.NewMockRequestLogStore(ctrl)
234-
logStore.EXPECT().List(gomock.Any(), "q/1").Return(nil, fmt.Errorf("log backend down"))
224+
store := storagemock.NewMockStorage(ctrl)
225+
summaryStore := storagemock.NewMockRequestSummaryStore(ctrl)
226+
store.EXPECT().GetRequestSummaryStore().Return(summaryStore)
227+
summaryStore.EXPECT().Get(gomock.Any(), "q/1").Return(entity.RequestSummary{}, fmt.Errorf("summary backend down"))
235228

236229
registry, publisher := newCancelTestRegistry(t, ctrl)
237230
_ = publisher
238231

239-
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, logStore, registry)
232+
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, store, registry)
240233
err := controller.Cancel(context.Background(), testCancelRequest("q/1", ""))
241234
require.Error(t, err)
242235
assert.False(t, errs.IsUserError(err))

submitqueue/gateway/controller/land.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,8 @@ type LandController struct {
6666
logger *zap.SugaredLogger
6767
metricsScope tally.Scope
6868
counter counter.Counter
69-
store storage.Storage
7069
receiptWriter *requestcore.ReceiptWriter
70+
materializer *requestcore.Materializer
7171
queueConfigs queueconfig.Store
7272
registry consumer.TopicRegistry
7373
}
@@ -80,8 +80,8 @@ func NewLandController(logger *zap.SugaredLogger, scope tally.Scope, counter cou
8080
logger: logger,
8181
metricsScope: scope.SubScope("land_controller"),
8282
counter: counter,
83-
store: store,
8483
receiptWriter: requestcore.NewReceiptWriter(store),
84+
materializer: requestcore.NewMaterializer(store),
8585
queueConfigs: queueConfigs,
8686
registry: registry,
8787
}
@@ -148,7 +148,7 @@ func (c *LandController) Land(ctx context.Context, req entity.LandRequest) (resu
148148
Status: entity.RequestStatusAccepted,
149149
Metadata: map[string]string{},
150150
}
151-
if err := c.store.GetRequestLogStore().Insert(ctx, logEntry); err != nil {
151+
if err := c.materializer.PersistLog(ctx, logEntry); err != nil {
152152
// Publication is the Land success boundary. Returning an error here would
153153
// encourage the caller to submit a duplicate request that is already queued.
154154
c.logger.Errorw("failed to record accepted status after publishing request",

0 commit comments

Comments
 (0)