Skip to content

Commit 3b2b897

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 6ec424a commit 3b2b897

24 files changed

Lines changed: 242 additions & 168 deletions

File tree

service/submitqueue/gateway/server/main.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ func run() error {
252252
// Create controllers and wrap them for gRPC
253253
pingController := controller.NewPingController(logger, scope)
254254
landController := controller.NewLandController(logger.Sugar(), scope, cnt, store, queueConfigs, registry)
255-
cancelController := controller.NewCancelController(logger.Sugar(), scope, requestLogStore, registry)
255+
cancelController := controller.NewCancelController(logger.Sugar(), scope, store, registry)
256256
statusController := controller.NewStatusController(logger.Sugar(), scope, requestLogStore)
257257
gatewayServer := &GatewayServer{
258258
pingController: pingController,

submitqueue/gateway/controller/cancel.go

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

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

@@ -84,18 +86,13 @@ func (c *CancelController) Cancel(ctx context.Context, req *pb.CancelRequest) (*
8486
"reason", cancelRequest.Reason,
8587
)
8688

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

10198
// Record the user's intent in the request log before publishing. Writing direct to the
@@ -106,7 +103,7 @@ func (c *CancelController) Cancel(ctx context.Context, req *pb.CancelRequest) (*
106103
metadata["reason"] = cancelRequest.Reason
107104
}
108105
logEntry := entity.NewRequestLog(cancelRequest.ID, entity.RequestStatusCancelling, 0, "", metadata)
109-
if err := c.requestLogStore.Insert(ctx, logEntry); err != nil {
106+
if err := c.materializer.PersistLog(ctx, logEntry); err != nil {
110107
return nil, fmt.Errorf("CancelController failed to insert cancelling log for sqid=%s: %w", cancelRequest.ID, err)
111108
}
112109

submitqueue/gateway/controller/cancel_test.go

Lines changed: 41 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ import (
2929
queuemock "github.com/uber/submitqueue/platform/extension/messagequeue/mock"
3030
"github.com/uber/submitqueue/submitqueue/core/topickey"
3131
"github.com/uber/submitqueue/submitqueue/entity"
32-
"github.com/uber/submitqueue/submitqueue/extension/storage"
3332
storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock"
3433
"go.uber.org/mock/gomock"
3534
"go.uber.org/zap"
@@ -58,27 +57,29 @@ func newCancelTestRegistryWithNoopPublisher(t *testing.T, ctrl *gomock.Controlle
5857
return registry
5958
}
6059

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

7172
func TestNewCancelController(t *testing.T) {
7273
ctrl := gomock.NewController(t)
7374

74-
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newRequestLogStoreNoop(t, ctrl), newCancelTestRegistryWithNoopPublisher(t, ctrl))
75+
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newCancelStorageFixture(ctrl, "test-queue/42").storage, newCancelTestRegistryWithNoopPublisher(t, ctrl))
7576
require.NotNil(t, controller)
7677
}
7778

7879
func TestCancel_HappyPath(t *testing.T) {
7980
ctrl := gomock.NewController(t)
8081

81-
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newRequestLogStoreNoop(t, ctrl), newCancelTestRegistryWithNoopPublisher(t, ctrl))
82+
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newCancelStorageFixture(ctrl, "test-queue/42").storage, newCancelTestRegistryWithNoopPublisher(t, ctrl))
8283
ctx := context.Background()
8384

8485
req := &pb.CancelRequest{Sqid: "test-queue/42", Reason: "user changed their mind"}
@@ -91,7 +92,7 @@ func TestCancel_HappyPath(t *testing.T) {
9192
func TestCancel_ReturnsErrorOnEmptySqid(t *testing.T) {
9293
ctrl := gomock.NewController(t)
9394

94-
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newRequestLogStoreNoop(t, ctrl), newCancelTestRegistryWithNoopPublisher(t, ctrl))
95+
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newCancelStorageFixture(ctrl, "test-queue/42").storage, newCancelTestRegistryWithNoopPublisher(t, ctrl))
9596
ctx := context.Background()
9697

9798
req := &pb.CancelRequest{Sqid: "", Reason: "anything"}
@@ -116,7 +117,7 @@ func TestCancel_PublishesToQueue(t *testing.T) {
116117
},
117118
)
118119

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

122123
req := &pb.CancelRequest{Sqid: "my-queue/7", Reason: "obsolete change"}
@@ -133,91 +134,57 @@ func TestCancel_PublishesToQueue(t *testing.T) {
133134
assert.Equal(t, "obsolete change", deserialized.Reason)
134135
}
135136

136-
// TestCancel_InsertsCancellingLog asserts that Cancel records a RequestStatusCancelling
137-
// log entry (intent) carrying the reason in metadata, and that the entry is written
138-
// before the cancel topic publish so observers see intent the moment Cancel returns.
139-
func TestCancel_InsertsCancellingLog(t *testing.T) {
137+
// TestCancel_MaterializesCancellingBeforePublish asserts that Cancel records the intent before publishing.
138+
func TestCancel_MaterializesCancellingBeforePublish(t *testing.T) {
140139
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)
140+
fixture := newCancelStorageFixture(ctrl, "my-queue/42")
151141

152142
registry, publisher := newCancelTestRegistry(t, ctrl)
153-
insertedBeforePublish := false
154143
publisher.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(
155144
func(_ context.Context, _ string, _ entityqueue.Message) error {
156-
insertedBeforePublish = insertedLog.RequestID != ""
145+
summary, err := fixture.summaryStore.Get(context.Background(), "my-queue/42")
146+
require.NoError(t, err)
147+
assert.Equal(t, entity.RequestStatusCancelling, summary.Status)
148+
assert.Equal(t, "obsolete change", summary.Metadata["reason"])
157149
return nil
158150
},
159151
)
160152

161-
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, logStore, registry)
162-
163-
req := &pb.CancelRequest{Sqid: "my-queue/42", Reason: "obsolete change"}
164-
_, err := controller.Cancel(context.Background(), req)
153+
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, fixture.storage, registry)
154+
_, err := controller.Cancel(context.Background(), &pb.CancelRequest{Sqid: "my-queue/42", Reason: "obsolete change"})
165155
require.NoError(t, err)
166-
167-
assert.Equal(t, "my-queue/42", insertedLog.RequestID)
168-
assert.Equal(t, entity.RequestStatusCancelling, insertedLog.Status)
169-
assert.Equal(t, "obsolete change", insertedLog.Metadata["reason"])
170-
assert.True(t, insertedBeforePublish, "log entry must be inserted before publish to the cancel topic")
171156
}
172157

173-
// TestCancel_LogInsertFailure asserts that a failure to insert the Cancelling log entry
174-
// short-circuits the RPC with an error and the cancel topic is never published to.
175-
func TestCancel_LogInsertFailure(t *testing.T) {
158+
func TestCancel_LogInsertFailureSkipsPublish(t *testing.T) {
176159
ctrl := gomock.NewController(t)
177-
178-
logStore := storagemock.NewMockRequestLogStore(ctrl)
179-
logStore.EXPECT().List(gomock.Any(), "q/1").Return([]entity.RequestLog{{}}, nil)
180-
logStore.EXPECT().Insert(gomock.Any(), gomock.Any()).Return(fmt.Errorf("db unavailable"))
181-
160+
fixture := newCancelStorageFixture(ctrl, "q/1")
161+
fixture.setLogInsertError(fmt.Errorf("db unavailable"))
182162
registry, publisher := newCancelTestRegistry(t, ctrl)
183-
// No Publish expectation: log insert must fail before publish runs.
184163
_ = publisher
185164

186-
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, logStore, registry)
165+
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, fixture.storage, registry)
187166
_, err := controller.Cancel(context.Background(), &pb.CancelRequest{Sqid: "q/1"})
188167
require.Error(t, err)
189168
}
190169

191170
func TestCancel_ReturnsErrorOnPublishFailure(t *testing.T) {
192171
ctrl := gomock.NewController(t)
193-
172+
fixture := newCancelStorageFixture(ctrl, "test-queue/1")
194173
registry, publisher := newCancelTestRegistry(t, ctrl)
195174
publisher.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("queue unavailable"))
196175

197-
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, newRequestLogStoreNoop(t, ctrl), registry)
198-
ctx := context.Background()
199-
200-
req := &pb.CancelRequest{Sqid: "test-queue/1"}
201-
_, err := controller.Cancel(ctx, req)
202-
176+
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, fixture.storage, registry)
177+
_, err := controller.Cancel(context.Background(), &pb.CancelRequest{Sqid: "test-queue/1"})
203178
require.Error(t, err)
204179
}
205180

206-
// TestCancel_UnknownSqidIsUserError asserts that Cancel for a sqid with no
207-
// request_log history fails fast with a RequestNotFoundError (user error) and
208-
// never inserts a cancelling log row or publishes to the cancel topic.
209181
func TestCancel_UnknownSqidIsUserError(t *testing.T) {
210182
ctrl := gomock.NewController(t)
211-
212-
logStore := storagemock.NewMockRequestLogStore(ctrl)
213-
logStore.EXPECT().List(gomock.Any(), "ghost/1").Return(nil, storage.ErrNotFound)
214-
// No Insert expectation: existence check must short-circuit before Insert.
215-
183+
fixture := newCancelStorageFixture(ctrl, "")
216184
registry, publisher := newCancelTestRegistry(t, ctrl)
217-
// No Publish expectation: existence check must short-circuit before Publish.
218185
_ = publisher
219186

220-
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, logStore, registry)
187+
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, fixture.storage, registry)
221188
_, err := controller.Cancel(context.Background(), &pb.CancelRequest{Sqid: "ghost/1"})
222189
require.Error(t, err)
223190
assert.True(t, IsRequestNotFound(err))
@@ -229,19 +196,19 @@ func TestCancel_UnknownSqidIsUserError(t *testing.T) {
229196
assert.Equal(t, "ghost/1", typed.Sqid)
230197
}
231198

232-
// TestCancel_RequestLogLookupFailure asserts that an infrastructure failure on
233-
// the existence check propagates as a (non-user) error and skips the rest of
234-
// the pipeline.
235-
func TestCancel_RequestLogLookupFailure(t *testing.T) {
199+
func TestCancel_RequestSummaryLookupFailure(t *testing.T) {
236200
ctrl := gomock.NewController(t)
237-
238-
logStore := storagemock.NewMockRequestLogStore(ctrl)
239-
logStore.EXPECT().List(gomock.Any(), "q/1").Return(nil, fmt.Errorf("log backend down"))
240-
201+
store := storagemock.NewMockStorage(ctrl)
202+
summaryStore := storagemock.NewMockRequestSummaryStore(ctrl)
203+
store.EXPECT().GetRequestSummaryStore().Return(summaryStore).AnyTimes()
204+
store.EXPECT().GetRequestLogStore().Return(storagemock.NewMockRequestLogStore(ctrl)).AnyTimes()
205+
store.EXPECT().GetRequestQueueSummaryStore().Return(storagemock.NewMockRequestQueueSummaryStore(ctrl)).AnyTimes()
206+
store.EXPECT().GetRequestURIStore().Return(storagemock.NewMockRequestURIStore(ctrl)).AnyTimes()
207+
summaryStore.EXPECT().Get(gomock.Any(), "q/1").Return(entity.RequestSummary{}, fmt.Errorf("summary backend down"))
241208
registry, publisher := newCancelTestRegistry(t, ctrl)
242209
_ = publisher
243210

244-
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, logStore, registry)
211+
controller := NewCancelController(zap.NewNop().Sugar(), tally.NoopScope, store, registry)
245212
_, err := controller.Cancel(context.Background(), &pb.CancelRequest{Sqid: "q/1"})
246213
require.Error(t, err)
247214
assert.False(t, errs.IsUserError(err))

submitqueue/gateway/controller/land.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,8 @@ type LandController struct {
7272
logger *zap.SugaredLogger
7373
metricsScope tally.Scope
7474
counter counter.Counter
75-
store storage.Storage
7675
admissionWriter *requestcore.AdmissionWriter
76+
materializer *requestcore.Materializer
7777
queueConfigs queueconfig.Store
7878
registry consumer.TopicRegistry
7979
}
@@ -86,8 +86,8 @@ func NewLandController(logger *zap.SugaredLogger, scope tally.Scope, counter cou
8686
logger: logger,
8787
metricsScope: scope.SubScope("land_controller"),
8888
counter: counter,
89-
store: store,
9089
admissionWriter: requestcore.NewAdmissionWriter(store),
90+
materializer: requestcore.NewMaterializer(store),
9191
queueConfigs: queueConfigs,
9292
registry: registry,
9393
}
@@ -168,8 +168,8 @@ func (c *LandController) Land(ctx context.Context, req *pb.LandRequest) (resp *p
168168
Status: entity.RequestStatusAccepted,
169169
Metadata: map[string]string{},
170170
}
171-
if err := c.store.GetRequestLogStore().Insert(ctx, logEntry); err != nil {
172-
return nil, fmt.Errorf("LandController failed to insert request log for sqid=%s: %w", landRequest.ID, err)
171+
if err := c.materializer.PersistLog(ctx, logEntry); err != nil {
172+
return nil, fmt.Errorf("LandController failed to persist accepted status for sqid=%s: %w", landRequest.ID, err)
173173
}
174174

175175
c.logger.Debugw("land request created",

submitqueue/gateway/controller/land_test.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,16 @@ func TestLand_PublishesToQueue(t *testing.T) {
330330
return nil
331331
},
332332
),
333+
summaryStore.EXPECT().Get(gomock.Any(), "test-queue/123").DoAndReturn(
334+
func(context.Context, string) (entity.RequestSummary, error) {
335+
return persistedSummary, nil
336+
},
337+
),
338+
queueStore.EXPECT().Get(gomock.Any(), "test-queue", gomock.Any(), "test-queue/123").DoAndReturn(
339+
func(context.Context, string, int64, string) (entity.RequestQueueSummary, error) {
340+
return persistedQueueSummary, nil
341+
},
342+
),
333343
publisher.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(
334344
func(ctx context.Context, topic string, msg entityqueue.Message) error {
335345
publishedTopic = topic

submitqueue/gateway/controller/log/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ go_library(
88
deps = [
99
"//platform/consumer:go_default_library",
1010
"//platform/metrics:go_default_library",
11+
"//submitqueue/core/request:go_default_library",
1112
"//submitqueue/entity:go_default_library",
1213
"//submitqueue/extension/storage:go_default_library",
1314
"@com_github_uber_go_tally//:go_default_library",

submitqueue/gateway/controller/log/log.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"github.com/uber-go/tally"
2222
"github.com/uber/submitqueue/platform/consumer"
2323
"github.com/uber/submitqueue/platform/metrics"
24+
requestcore "github.com/uber/submitqueue/submitqueue/core/request"
2425
"github.com/uber/submitqueue/submitqueue/entity"
2526
"github.com/uber/submitqueue/submitqueue/extension/storage"
2627
"go.uber.org/zap"
@@ -36,7 +37,7 @@ import (
3637
type Controller struct {
3738
logger *zap.SugaredLogger
3839
metricsScope tally.Scope
39-
store storage.Storage
40+
materializer *requestcore.Materializer
4041
topicKey consumer.TopicKey
4142
consumerGroup string
4243
}
@@ -55,7 +56,7 @@ func NewController(
5556
return &Controller{
5657
logger: logger.Named("log_controller"),
5758
metricsScope: scope.SubScope("log_controller"),
58-
store: store,
59+
materializer: requestcore.NewMaterializer(store),
5960
topicKey: topicKey,
6061
consumerGroup: consumerGroup,
6162
}
@@ -87,10 +88,10 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r
8788
"attempt", delivery.Attempt(),
8889
)
8990

90-
// Persist request log to storage
91-
if err := c.store.GetRequestLogStore().Insert(ctx, logEntry); err != nil {
91+
// Persist the audit log and materialized views as one retryable application operation.
92+
if err := c.materializer.PersistLog(ctx, logEntry); err != nil {
9293
metrics.NamedCounter(c.metricsScope, opName, "storage_errors", 1)
93-
return fmt.Errorf("failed to insert request log: %w", err)
94+
return fmt.Errorf("failed to persist request log: %w", err)
9495
}
9596

9697
return nil // Success - message will be acked

0 commit comments

Comments
 (0)