Skip to content

Commit 1ce24e4

Browse files
authored
refactor(submitqueue): remove duplicate List projection
Summary: Intent: - Complete the receipt-based List migration from #800: keep one authoritative request summary and immutable lookup keys. Changes: - Remove queue-summary writes, the duplicate entity/store, and obsolete tests and mocks. - Idempotently ensure URI mappings before the receipt key, including partial-write retries and stale logs. - Update rollout docs; retain the legacy SQL definition for deployment compatibility. Test Plan: - Deploy after #800 is stable. Rollback must retain the receipt-based reader once duplicate writes stop. - Retire the table in a separate internal schema change after old binaries and the rollback window are gone. UQL DEPRECATED can drop immediately in Vitess; no physical retirement is enabled here. --- <sub>Generated by the 🪄 [pr-create](https://sg.uberinternal.com/code.uber.internal/uber-code/devexp-agent-marketplace/-/blob/claude-code/plugins/dev/uber-dev/skills/pr-create/SKILL.md) skill in devexp-agent-marketplace</sub>
1 parent 8f30300 commit 1ce24e4

24 files changed

Lines changed: 160 additions & 1234 deletions

‎doc/rfc/service-scoped-extensions.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ Every store, verified against actual usage rather than intent.
3838
|---|---|
3939
| `RequestLogStore` | `request_log` |
4040
| `RequestSummaryStore` | `request_summary` |
41-
| `RequestQueueSummaryStore` | `request_summary_by_queue` |
41+
| `RequestReceiptStore` | `request_receipt` |
4242
| `RequestURIStore` | `change_uri_request_mapping` |
4343

4444
| Orchestrator | Table |

‎doc/rfc/submitqueue/status-list-api.md‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ The key supports authoritative lookup and conditional status updates for one req
9191

9292
The immutable `request_receipt` lookup contains only `(queue, received_at_ms, request_id)` as its primary key and supports a bounded descending scan over `(received_at_ms, request_id)`. Request IDs break timestamp ties in string order, not numeric order. A backend may use a reverse range scan or descending-encode the ordered key components; cursors always carry the original receipt time and request ID.
9393

94-
List resolves these keys through `request_summary` rather than storing response fields in the lookup. The old `request_summary_by_queue` projection still receives writes for rollback, but List no longer reads it.
94+
List resolves these keys through `request_summary` rather than storing response fields in the lookup. The old `request_summary_by_queue` projection is no longer read or written; its SQL definition is retained only for deployment compatibility until physical retirement.
9595

9696
The logical key covers the `List` queue predicate, receipt-time range, newest-first ordering, and complete keyset cursor in one bounded scan.
9797

@@ -109,17 +109,17 @@ The URI is stored in the canonical form received from the validated Land request
109109

110110
### Land Receipt
111111

112-
After synchronous validation, Land generates the sqid and one receipt timestamp. The gateway persists the authoritative summary in the internal `accepting` state, then publishes the request to the orchestrator. It does not create the URI or queue projections while the request remains `accepting`.
112+
After synchronous validation, Land generates the sqid and one receipt timestamp. The gateway persists the authoritative summary in the internal `accepting` state, then publishes the request to the orchestrator. It does not create the URI or receipt mappings while the request remains `accepting`.
113113

114-
After publication succeeds, Land appends the initial `accepted` request log. Materializing `accepted`, `started`, or any later lifecycle state promotes the authoritative summary out of `accepting`, creates the URI and legacy queue projections, and ensures the receipt lookup key exists. This handles the race where the orchestrator emits `started` before Land finishes persisting `accepted`.
114+
After publication succeeds, Land appends the initial `accepted` request log. Materializing `accepted`, `started`, or any later lifecycle state promotes the authoritative summary out of `accepting`, ensures each immutable URI mapping, then ensures the receipt lookup key exists. This handles the race where the orchestrator emits `started` before Land finishes persisting `accepted`.
115115

116116
An `accepted` event is the lowest public lifecycle state. A late `accepted` event is retained in request history but must not replace `started` or any later materialized status.
117117

118118
Pipeline publication is the Land success boundary. If publication fails, Land returns an error and leaves the hidden `accepting` receipt for operational cleanup. If publication succeeds but appending or materializing `accepted` fails, Land still returns the sqid because retrying the RPC would submit a duplicate request that is already in the pipeline. A later pipeline event activates and repairs the public projections.
119119

120120
### Request-Log Materialization
121121

122-
Every gateway request-log persistence path uses the same materialization component. It appends the audit log, compares the incoming entry with the authoritative summary, and conditionally advances the winner. For public summaries, it completes the URI and legacy queue projection writes before ensuring the immutable receipt key, including on unchanged or stale-log retries. Hidden `accepting` summaries remain excluded; a failed receipt write is returned through the existing projection-failure path.
122+
Every gateway request-log persistence path uses the same materialization component. It appends the audit log, compares the incoming entry with the authoritative summary, and conditionally advances the winner. For public summaries, it idempotently ensures every URI mapping before ensuring the receipt key, including on unchanged or stale-log retries. No mutable response state is copied to lookup tables. Hidden `accepting` summaries remain excluded; lookup-write failures are returned for retry.
123123

124124
The winner comparison preserves the existing current-status reconciliation behavior:
125125

@@ -172,7 +172,9 @@ CD must roll out the additive table and receipt writers to all instances before
172172

173173
There is no historical backfill or fallback to the legacy queue projection. Requests without receipt keys are absent from List, even if their old queue-summary rows remain. Later log activity can create missing receipt keys.
174174

175-
Keep legacy projection writes during the rollback window; reverting the read cutover restores the old List path. Once stable, a follow-up stops duplicate projection writes and removes `RequestQueueSummary` and its store. Retire `request_summary_by_queue` after the rollback window. The final model keeps one full projection plus immutable lookup keys.
175+
After the read cutover is stable, deploy the cleanup that stops duplicate writes and removes `RequestQueueSummary` and its store. Rollback remains safe to the receipt-based reader with dual writes; reverting to the old List reader is no longer safe once duplicate writes stop, because the old table is stale and missing newer requests. Restoring that reader would also require restoring writers and reconciling missed data.
176+
177+
Keep the legacy SQL table definition through code rollout and the rollback window. Physical retirement is a separate internal schema change after all old binaries are gone. UQL's table-level `DEPRECATED` annotation initiates retirement (rename in MySQL, potentially immediate drop in Vitess), so do not add it as part of this code cleanup. See the [gateway schema retirement notes](../../../submitqueue/gateway/extension/storage/mysql/schema/README.md#legacy-table-retirement). The final model keeps one full projection plus immutable lookup keys.
176178

177179
## Rejected Alternatives
178180

‎submitqueue/entity/request_summary.go‎

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -57,26 +57,6 @@ type RequestSummary struct {
5757
Metadata map[string]string
5858
}
5959

60-
// RequestQueueSummary is a queue-ordered copy of a public request summary.
61-
type RequestQueueSummary struct {
62-
// RequestID is the canonical decimal request identifier, unique within Queue.
63-
RequestID string
64-
// Queue is the queue supplied at receipt.
65-
Queue string
66-
// ChangeURIs are the change URIs supplied at receipt in caller order.
67-
ChangeURIs []string
68-
// ReceivedAtMs is the immutable receipt timestamp in Unix milliseconds.
69-
ReceivedAtMs int64
70-
// Status is the current customer-facing request status.
71-
Status RequestStatus
72-
// Version is copied from the authoritative RequestSummary and guards stale projection writers.
73-
Version int32
74-
// LastError is the error associated with the current status, or empty when absent.
75-
LastError string
76-
// Metadata is display and debugging metadata associated with the current status.
77-
Metadata map[string]string
78-
}
79-
8060
// RequestURI maps one change URI to one received request.
8161
type RequestURI struct {
8262
// ChangeURI is the exact canonical URI supplied at receipt.

‎submitqueue/extension/storage/BUILD.bazel‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ go_library(
1111
"queue_batch_state_store.go",
1212
"request_batch_store.go",
1313
"request_log_store.go",
14-
"request_queue_summary_store.go",
1514
"request_receipt_store.go",
1615
"request_store.go",
1716
"request_summary_store.go",

‎submitqueue/extension/storage/mock/BUILD.bazel‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ go_library(
1111
"queue_batch_state_store_mock.go",
1212
"request_batch_store_mock.go",
1313
"request_log_store_mock.go",
14-
"request_queue_summary_store_mock.go",
1514
"request_receipt_store_mock.go",
1615
"request_store_mock.go",
1716
"request_summary_store_mock.go",

‎submitqueue/extension/storage/mock/request_queue_summary_store_mock.go‎

Lines changed: 0 additions & 101 deletions
This file was deleted.

‎submitqueue/extension/storage/request_queue_summary_store.go‎

Lines changed: 0 additions & 63 deletions
This file was deleted.

‎submitqueue/gateway/controller/land_test.go‎

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@ import (
3636
"github.com/uber/submitqueue/submitqueue/entity"
3737
"github.com/uber/submitqueue/submitqueue/extension/queueconfig"
3838
qcmock "github.com/uber/submitqueue/submitqueue/extension/queueconfig/mock"
39-
basestorage "github.com/uber/submitqueue/submitqueue/extension/storage"
4039
storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock"
4140
requestcore "github.com/uber/submitqueue/submitqueue/gateway/core/request"
4241
gwstoragemock "github.com/uber/submitqueue/submitqueue/gateway/extension/storage/mock"
@@ -347,7 +346,6 @@ func TestLand_PublishesToQueue(t *testing.T) {
347346
var receiptSummary entity.RequestSummary
348347
var materializedSummary entity.RequestSummary
349348
var persistedMapping entity.RequestURI
350-
var persistedQueueSummary entity.RequestQueueSummary
351349
var persistedReceipt entity.RequestReceipt
352350
var persistedLog entity.RequestLog
353351

@@ -359,10 +357,8 @@ func TestLand_PublishesToQueue(t *testing.T) {
359357
store := gwstoragemock.NewMockStorage(ctrl)
360358
summaryStore := storagemock.NewMockRequestSummaryStore(ctrl)
361359
uriStore := storagemock.NewMockRequestURIStore(ctrl)
362-
queueStore := storagemock.NewMockRequestQueueSummaryStore(ctrl)
363360
receiptStore := storagemock.NewMockRequestReceiptStore(ctrl)
364361
logStore := storagemock.NewMockRequestLogStore(ctrl)
365-
store.EXPECT().GetRequestQueueSummaryStore().Return(queueStore).AnyTimes()
366362
store.EXPECT().GetRequestReceiptStore().Return(receiptStore).AnyTimes()
367363
store.EXPECT().GetRequestSummaryStore().Return(summaryStore).AnyTimes()
368364
store.EXPECT().GetRequestLogStore().Return(logStore).AnyTimes()
@@ -404,23 +400,12 @@ func TestLand_PublishesToQueue(t *testing.T) {
404400
return nil
405401
},
406402
),
407-
queueStore.EXPECT().Get(gomock.Any(), gomock.Any(), "123").DoAndReturn(
408-
func(context.Context, int64, string) (entity.RequestQueueSummary, error) {
409-
return entity.RequestQueueSummary{}, basestorage.ErrNotFound
410-
},
411-
),
412403
uriStore.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn(
413404
func(_ context.Context, mapping entity.RequestURI) error {
414405
persistedMapping = mapping
415406
return nil
416407
},
417408
),
418-
queueStore.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn(
419-
func(_ context.Context, summary entity.RequestQueueSummary) error {
420-
persistedQueueSummary = summary
421-
return nil
422-
},
423-
),
424409
receiptStore.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn(
425410
func(_ context.Context, receipt entity.RequestReceipt) error {
426411
persistedReceipt = receipt
@@ -470,15 +455,6 @@ func TestLand_PublishesToQueue(t *testing.T) {
470455
ReceivedAtMs: receiptSummary.ReceivedAtMs,
471456
RequestID: "123",
472457
}, persistedMapping)
473-
assert.Equal(t, entity.RequestQueueSummary{
474-
RequestID: "123",
475-
Queue: "test-queue",
476-
ChangeURIs: []string{"github://github.example.com/uber/backend/pull/456/fedcba9876543210fedcba9876543210fedcba98"},
477-
ReceivedAtMs: receiptSummary.ReceivedAtMs,
478-
Status: entity.RequestStatusAccepted,
479-
Version: 2,
480-
Metadata: map[string]string{},
481-
}, persistedQueueSummary)
482458

483459
// Verify message was published to the topic registered under TopicKeyStart
484460
assert.Equal(t, "start", publishedTopic)

0 commit comments

Comments
 (0)