Skip to content

Commit aabd434

Browse files
albertywuclaude
andauthored
feat(orchestrator): emit batched request log from batch controller (#287)
## Summary Previously the batch controller was not publishing a request log on completion. This is inconsistent with the other controllers, which publish a terminal request log when its action is complete. This PR emits a request log after the batch is persisted and before publishing to score. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> ## Test Plan ✅ make fmt && make build && make test && make check-mocks && make e2e-test ## Issues ## Stack 1. @ #287 1. #288 1. #289 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 631f778 commit aabd434

3 files changed

Lines changed: 97 additions & 3 deletions

File tree

submitqueue/orchestrator/controller/batch/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ go_library(
1010
"//platform/consumer",
1111
"//platform/extension/counter",
1212
"//platform/metrics",
13+
"//submitqueue/core/request",
1314
"//submitqueue/core/topickey",
1415
"//submitqueue/entity",
1516
"//submitqueue/extension/conflict",

submitqueue/orchestrator/controller/batch/batch.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
"github.com/uber/submitqueue/platform/consumer"
2525
"github.com/uber/submitqueue/platform/extension/counter"
2626
"github.com/uber/submitqueue/platform/metrics"
27+
corerequest "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/conflict"
@@ -287,6 +288,20 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r
287288
"dependency_count", len(batch.Dependencies),
288289
)
289290

291+
// Record the "batched" status in the request log. This status corresponds to
292+
// the RequestStateBatched transition CAS'd above, so it carries the request
293+
// version for reconciliation (unlike the batch-level "scored" status). The
294+
// message ID is scoped to (requestID, status), so a redelivery that creates a
295+
// fresh batch re-emits "batched" with a different batch_id but is deduped to
296+
// the first entry — acceptable, the request is batched either way.
297+
logEntry := entity.NewRequestLog(request.ID, entity.RequestStatusBatched, request.Version, "", map[string]string{
298+
"batch_id": batch.ID,
299+
})
300+
if err := corerequest.PublishLog(ctx, c.registry, logEntry, request.ID); err != nil {
301+
metrics.NamedCounter(c.metricsScope, opName, "request_log_errors", 1)
302+
return fmt.Errorf("failed to publish request log for request %s: %w", request.ID, err)
303+
}
304+
290305
// Publish to score topic for further processing.
291306
// If it fails and the controller retries, a new batch will be created with the new batch ID but the same request ID.
292307
// The downstream logic should be able to handle stale entries by looking at the state of the batch.

submitqueue/orchestrator/controller/batch/batch_test.go

Lines changed: 81 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,10 @@ func testRequest() entity.Request {
7575
// newTestController creates a controller with test dependencies.
7676
// If mockStorage is nil, a default MockStorage with an empty batch store is created.
7777
// If analyzer is nil, the "all" conflict analyzer is used (every active batch becomes a dependency).
78-
func newTestController(t *testing.T, ctrl *gomock.Controller, cnt *countermock.MockCounter, mockStorage *storagemock.MockStorage, analyzer conflict.Analyzer, publishErr error) *Controller {
78+
// scorePublishErr, if non-nil, is returned only for publishes to the "score" topic; the
79+
// log publish (which the controller emits first) always succeeds, so callers exercising the
80+
// score publish-failure path are not short-circuited on the earlier log publish.
81+
func newTestController(t *testing.T, ctrl *gomock.Controller, cnt *countermock.MockCounter, mockStorage *storagemock.MockStorage, analyzer conflict.Analyzer, scorePublishErr error) *Controller {
7982
logger := zaptest.NewLogger(t).Sugar()
8083
scope := tally.NoopScope
8184

@@ -105,15 +108,21 @@ func newTestController(t *testing.T, ctrl *gomock.Controller, cnt *countermock.M
105108
mockPub := queuemock.NewMockPublisher(ctrl)
106109
mockPub.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(
107110
func(ctx context.Context, topic string, msg entityqueue.Message) error {
108-
return publishErr
111+
if topic == "score" {
112+
return scorePublishErr
113+
}
114+
return nil
109115
},
110116
).AnyTimes()
111117

112118
mockQ := queuemock.NewMockQueue(ctrl)
113119
mockQ.EXPECT().Publisher().Return(mockPub).AnyTimes()
114120

115121
registry, err := consumer.NewTopicRegistry(
116-
[]consumer.TopicConfig{{Key: topickey.TopicKeyScore, Name: "score", Queue: mockQ}},
122+
[]consumer.TopicConfig{
123+
{Key: topickey.TopicKeyScore, Name: "score", Queue: mockQ},
124+
{Key: topickey.TopicKeyLog, Name: "log", Queue: mockQ},
125+
},
117126
)
118127
require.NoError(t, err)
119128

@@ -148,6 +157,75 @@ func TestController_Process_Success(t *testing.T) {
148157
require.NoError(t, err)
149158
}
150159

160+
// TestController_Process_PublishesBatchedLog asserts the controller emits a
161+
// "batched" request log carrying the request ID, the post-CAS request version,
162+
// and the batch ID it was placed into.
163+
func TestController_Process_PublishesBatchedLog(t *testing.T) {
164+
ctrl := gomock.NewController(t)
165+
166+
request := testRequest()
167+
168+
mockBatchStore := storagemock.NewMockBatchStore(ctrl)
169+
mockBatchStore.EXPECT().GetByQueueAndStates(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()
170+
mockBatchStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil)
171+
172+
mockReqStore := storagemock.NewMockRequestStore(ctrl)
173+
mockReqStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil)
174+
mockReqStore.EXPECT().UpdateState(gomock.Any(), request.ID, request.Version, request.Version+1, entity.RequestStateBatched).Return(nil)
175+
176+
mockBatchDependentStore := storagemock.NewMockBatchDependentStore(ctrl)
177+
mockBatchDependentStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil)
178+
179+
mockStorage := storagemock.NewMockStorage(ctrl)
180+
mockStorage.EXPECT().GetBatchStore().Return(mockBatchStore).AnyTimes()
181+
mockStorage.EXPECT().GetBatchDependentStore().Return(mockBatchDependentStore).AnyTimes()
182+
mockStorage.EXPECT().GetRequestStore().Return(mockReqStore).AnyTimes()
183+
184+
// Capture messages published to the log topic.
185+
var logMsgs []entityqueue.Message
186+
mockPub := queuemock.NewMockPublisher(ctrl)
187+
mockPub.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(
188+
func(ctx context.Context, topic string, msg entityqueue.Message) error {
189+
if topic == "log" {
190+
logMsgs = append(logMsgs, msg)
191+
}
192+
return nil
193+
},
194+
).AnyTimes()
195+
mockQ := queuemock.NewMockQueue(ctrl)
196+
mockQ.EXPECT().Publisher().Return(mockPub).AnyTimes()
197+
198+
registry, err := consumer.NewTopicRegistry(
199+
[]consumer.TopicConfig{
200+
{Key: topickey.TopicKeyScore, Name: "score", Queue: mockQ},
201+
{Key: topickey.TopicKeyLog, Name: "log", Queue: mockQ},
202+
},
203+
)
204+
require.NoError(t, err)
205+
206+
analyzerFactory := conflictmock.NewMockFactory(ctrl)
207+
analyzerFactory.EXPECT().For(gomock.Any()).Return(all.New(), nil).AnyTimes()
208+
controller := NewController(
209+
zaptest.NewLogger(t).Sugar(), tally.NoopScope, registry, newSequentialCounter(ctrl),
210+
mockStorage, analyzerFactory, topickey.TopicKeyBatch, "orchestrator-batch",
211+
)
212+
213+
msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil)
214+
delivery := queuemock.NewMockDelivery(ctrl)
215+
delivery.EXPECT().Message().Return(msg).AnyTimes()
216+
delivery.EXPECT().Attempt().Return(1).AnyTimes()
217+
218+
require.NoError(t, controller.Process(context.Background(), delivery))
219+
220+
require.Len(t, logMsgs, 1)
221+
logEntry, err := entity.RequestLogFromBytes(logMsgs[0].Payload)
222+
require.NoError(t, err)
223+
assert.Equal(t, request.ID, logEntry.RequestID)
224+
assert.Equal(t, entity.RequestStatusBatched, logEntry.Status)
225+
assert.Equal(t, request.Version+1, logEntry.RequestVersion)
226+
assert.Equal(t, "test-queue/batch/1", logEntry.Metadata["batch_id"])
227+
}
228+
151229
func TestController_Process_StorageFailure(t *testing.T) {
152230
ctrl := gomock.NewController(t)
153231

0 commit comments

Comments
 (0)