Skip to content

Commit 96a8ce6

Browse files
authored
feat(stovepipe): recover record DLQ work (#623)
## Summary ### Intent - Complete Stovepipe DLQ coverage for failures that occur while recording a completed request. - Reuse the existing idempotent record reconciliation path instead of introducing a separate controller or request lifecycle state. - This PR builds on #619, which aligns the Stovepipe DLQ controllers with repository conventions. ### Changes - Register the existing record controller for the `record_dlq` topic with a distinct consumer group. - Derive record controller identity from its topic key so primary and DLQ instances have separate logging and metrics. - Cover the record-DLQ controller identity configuration. ## Test Plan - `./tool/bazel test //stovepipe/controller/record:go_default_test --test_output=errors` - `./tool/bazel build //service/stovepipe/server:stovepipe` - `make fmt` - `make gazelle` ## Revert Plan - Revert this PR to remove record-DLQ registration and restore the single record-controller identity.
1 parent eb208cf commit 96a8ce6

4 files changed

Lines changed: 57 additions & 29 deletions

File tree

service/stovepipe/server/main.go

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -282,15 +282,15 @@ func run() error {
282282
// Each factory is constructed once and threaded through every consumer of
283283
// it, so a real (stateful) backend introduced later is shared rather than
284284
// silently duplicated across controllers.
285-
scf := fakeSourceControlFactory{}
285+
sourceControl := fakeSourceControlFactory{}
286286
brf := fakeBuildRunnerFactory{}
287287

288288
storageFty := storageFactory{backend: store}
289-
primaryCount, err := registerPrimaryControllers(primaryConsumer, logger.Sugar(), scope, storageFty, registry, scf, brf)
289+
primaryCount, err := registerPrimaryControllers(primaryConsumer, logger.Sugar(), scope, storageFty, registry, sourceControl, brf)
290290
if err != nil {
291291
return err
292292
}
293-
dlqCount, err := registerDLQControllers(dlqConsumer, logger.Sugar(), scope, storageFty, registry)
293+
dlqCount, err := registerDLQControllers(dlqConsumer, logger.Sugar(), scope, storageFty, registry, sourceControl)
294294
if err != nil {
295295
return err
296296
}
@@ -318,7 +318,7 @@ func run() error {
318318
logger.Sugar(),
319319
scope,
320320
newInMemoryCounterFactory(),
321-
scf,
321+
sourceControl,
322322
storageFty,
323323
registry,
324324
)
@@ -398,7 +398,7 @@ func registerPrimaryControllers(
398398
scope tally.Scope,
399399
store storage.Factory,
400400
registry consumer.TopicRegistry,
401-
scf sourcecontrol.Factory,
401+
sourceControl sourcecontrol.Factory,
402402
brf buildrunner.Factory,
403403
) (int, error) {
404404
var count int
@@ -408,7 +408,7 @@ func registerPrimaryControllers(
408408
scope,
409409
store,
410410
queueconfigdefault.NewStore(),
411-
scf,
411+
sourceControl,
412412
registry,
413413
stovepipemq.TopicKeyProcess,
414414
"stovepipe-process",
@@ -430,7 +430,7 @@ func registerPrimaryControllers(
430430
}
431431
count++
432432

433-
recordController := record.NewController(logger, scope, store, scf, stovepipemq.TopicKeyRecord, "stovepipe-record")
433+
recordController := record.NewController(logger, scope, store, sourceControl, stovepipemq.TopicKeyRecord, "stovepipe-record")
434434
if err := c.Register(recordController); err != nil {
435435
return count, fmt.Errorf("failed to register record controller: %w", err)
436436
}
@@ -447,6 +447,7 @@ func registerDLQControllers(
447447
scope tally.Scope,
448448
store storage.Factory,
449449
registry consumer.TopicRegistry,
450+
sourceControl sourcecontrol.Factory,
450451
) (int, error) {
451452
var count int
452453

@@ -468,6 +469,12 @@ func registerDLQControllers(
468469
}
469470
count++
470471

472+
recordDLQController := record.NewController(logger, scope, store, sourceControl, dlq.TopicKey(stovepipemq.TopicKeyRecord), "stovepipe-record-dlq")
473+
if err := c.Register(recordDLQController); err != nil {
474+
return count, fmt.Errorf("failed to register record dlq controller: %w", err)
475+
}
476+
count++
477+
471478
return count, nil
472479
}
473480

@@ -529,6 +536,12 @@ func newTopicRegistry(q extqueue.Queue, subscriberName string) (consumer.TopicRe
529536
Queue: q,
530537
Subscription: extqueue.DLQSubscriptionConfig(subscriberName, "stovepipe-buildsignal-dlq"),
531538
},
539+
{
540+
Key: dlq.TopicKey(stovepipemq.TopicKeyRecord),
541+
Name: "record_dlq",
542+
Queue: q,
543+
Subscription: extqueue.DLQSubscriptionConfig(subscriberName, "stovepipe-record-dlq"),
544+
},
532545
})
533546
}
534547

stovepipe/controller/record/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ go_test(
2424
embed = [":go_default_library"],
2525
deps = [
2626
"//platform/base/messagequeue:go_default_library",
27+
"//platform/consumer:go_default_library",
2728
"//platform/consumer/mock:go_default_library",
2829
"//stovepipe/core/messagequeue:go_default_library",
2930
"//stovepipe/entity:go_default_library",

stovepipe/controller/record/record.go

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -44,12 +44,12 @@ import (
4444
// when that fact is green advances the queue's last-green bookmark and promotes
4545
// the commit. Implements consumer.Controller.
4646
type Controller struct {
47-
logger *zap.SugaredLogger
48-
metricsScope tally.Scope
49-
stores storage.Factory
50-
sourceControls sourcecontrol.Factory
51-
topicKey consumer.TopicKey
52-
consumerGroup string
47+
logger *zap.SugaredLogger
48+
metricsScope tally.Scope
49+
stores storage.Factory
50+
sourceControl sourcecontrol.Factory
51+
topicKey consumer.TopicKey
52+
consumerGroup string
5353
}
5454

5555
// Verify Controller implements consumer.Controller interface at compile time.
@@ -68,17 +68,18 @@ func NewController(
6868
logger *zap.SugaredLogger,
6969
scope tally.Scope,
7070
stores storage.Factory,
71-
sourceControls sourcecontrol.Factory,
71+
sourceControl sourcecontrol.Factory,
7272
topicKey consumer.TopicKey,
7373
consumerGroup string,
7474
) *Controller {
75+
name := string(topicKey) + "_controller"
7576
return &Controller{
76-
logger: logger.Named("record_controller"),
77-
metricsScope: scope.SubScope("record_controller"),
78-
stores: stores,
79-
sourceControls: sourceControls,
80-
topicKey: topicKey,
81-
consumerGroup: consumerGroup,
77+
logger: logger.Named(name),
78+
metricsScope: scope.SubScope(name),
79+
stores: stores,
80+
sourceControl: sourceControl,
81+
topicKey: topicKey,
82+
consumerGroup: consumerGroup,
8283
}
8384
}
8485

@@ -246,7 +247,7 @@ func (c *Controller) reportFailureDetectionLatency(ctx context.Context, request
246247
return
247248
}
248249

249-
sourceControl, err := c.sourceControls.For(sourcecontrol.Config{QueueName: request.Queue})
250+
sourceControl, err := c.sourceControl.For(sourcecontrol.Config{QueueName: request.Queue})
250251
if err != nil {
251252
c.failureDetectionUnobserved(request, "resolve_source_control", err)
252253
return
@@ -365,7 +366,7 @@ func (c *Controller) advanceLastGreen(ctx context.Context, store storage.Storage
365366
func (c *Controller) emitLastGreenTimestamp(ctx context.Context, request entity.Request) {
366367
queueTag := metrics.NewTag("queue", request.Queue)
367368

368-
sourceControl, err := c.sourceControls.For(sourcecontrol.Config{QueueName: request.Queue})
369+
sourceControl, err := c.sourceControl.For(sourcecontrol.Config{QueueName: request.Queue})
369370
if err != nil {
370371
metrics.NamedCounter(c.metricsScope, _opName, "last_green_timestamp_resolve_errors", 1, queueTag)
371372
c.logger.Warnw("failed to resolve source control to report the last green timestamp",
@@ -420,7 +421,7 @@ func (c *Controller) emitLastGreenTimestamp(ctx context.Context, request entity.
420421
// harmlessly. A commit that a rewritten history dropped from the ref cannot be
421422
// promoted by any retry, so that case is counted and skipped rather than failed.
422423
func (c *Controller) promote(ctx context.Context, request entity.Request) error {
423-
sc, err := c.sourceControls.For(sourcecontrol.Config{QueueName: request.Queue})
424+
sc, err := c.sourceControl.For(sourcecontrol.Config{QueueName: request.Queue})
424425
if err != nil {
425426
metrics.NamedCounter(c.metricsScope, _opName, "source_control_errors", 1,
426427
metrics.NewTag("stage", "resolve"),
@@ -477,7 +478,7 @@ func (c *Controller) loadRequest(ctx context.Context, store storage.Storage, id
477478

478479
// Name returns the controller name for logging and metrics.
479480
func (c *Controller) Name() string {
480-
return "record"
481+
return string(c.topicKey)
481482
}
482483

483484
// TopicKey returns the topic key this controller subscribes to.

stovepipe/controller/record/record_test.go

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
"github.com/stretchr/testify/require"
2525
"github.com/uber-go/tally"
2626
entityqueue "github.com/uber/submitqueue/platform/base/messagequeue"
27+
"github.com/uber/submitqueue/platform/consumer"
2728
consumermock "github.com/uber/submitqueue/platform/consumer/mock"
2829
stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue"
2930
"github.com/uber/submitqueue/stovepipe/entity"
@@ -94,6 +95,10 @@ func (failingSourceControlFactory) For(sourcecontrol.Config) (sourcecontrol.Sour
9495
}
9596

9697
func newController(t *testing.T, ctrl *gomock.Controller) (*Controller, recordMocks) {
98+
return newControllerForTopic(t, ctrl, stovepipemq.TopicKeyRecord, "stovepipe-record")
99+
}
100+
101+
func newControllerForTopic(t *testing.T, ctrl *gomock.Controller, topicKey consumer.TopicKey, consumerGroup string) (*Controller, recordMocks) {
97102
t.Helper()
98103

99104
scope := tally.NewTestScope("", nil)
@@ -115,12 +120,20 @@ func newController(t *testing.T, ctrl *gomock.Controller) (*Controller, recordMo
115120
scope,
116121
staticStorageFactory{store: store},
117122
staticSourceControlFactory{sourceControl: m.sourceControl},
118-
stovepipemq.TopicKeyRecord,
119-
"stovepipe-record",
123+
topicKey,
124+
consumerGroup,
120125
)
121126
return c, m
122127
}
123128

129+
func TestControllerIdentity(t *testing.T) {
130+
c, _ := newControllerForTopic(t, gomock.NewController(t), consumer.TopicKey("record_dlq"), "stovepipe-record-dlq")
131+
132+
assert.Equal(t, "record_dlq", c.Name())
133+
assert.Equal(t, consumer.TopicKey("record_dlq"), c.TopicKey())
134+
assert.Equal(t, "stovepipe-record-dlq", c.ConsumerGroup())
135+
}
136+
124137
func delivery(t *testing.T, ctrl *gomock.Controller, payload []byte) *consumermock.MockDelivery {
125138
t.Helper()
126139
d := consumermock.NewMockDelivery(ctrl)
@@ -283,7 +296,7 @@ func TestProcess_TimestampReportingFailureDoesNotFailRecord(t *testing.T) {
283296
func TestProcess_UnresolvableSourceControlCountsTimestampFailure(t *testing.T) {
284297
ctrl := gomock.NewController(t)
285298
c, m := newController(t, ctrl)
286-
c.sourceControls = failingSourceControlFactory{}
299+
c.sourceControl = failingSourceControlFactory{}
287300

288301
m.reqStore.EXPECT().Get(gomock.Any(), testID).
289302
Return(requestWithState(entity.RequestStateSucceeded), nil)
@@ -385,7 +398,7 @@ func TestProcess_UnobservableDetectionLatencyDoesNotFailRecord(t *testing.T) {
385398
name: "source control cannot be resolved",
386399
step: "resolve_source_control",
387400
setup: func(c *Controller, _ recordMocks) {
388-
c.sourceControls = failingSourceControlFactory{}
401+
c.sourceControl = failingSourceControlFactory{}
389402
},
390403
},
391404
{
@@ -566,7 +579,7 @@ func TestProcess_PromotionErrorsPropagate(t *testing.T) {
566579
{
567580
name: "source control resolve fails",
568581
setup: func(c *Controller, _ recordMocks) {
569-
c.sourceControls = failingSourceControlFactory{}
582+
c.sourceControl = failingSourceControlFactory{}
570583
},
571584
},
572585
{

0 commit comments

Comments
 (0)