Skip to content

Commit 30fb699

Browse files
committed
feat(messagequeue)!: remove PublishAfter and the visible_after column
## Summary ### Why? PublishAfter existed for one pattern: a consumer acking its own delivery and republishing a delayed copy to keep a poll or gate-wait loop alive. The two previous commits migrated all three such loops (stovepipe process/buildsignal, orchestrator buildsignal) to the hold primitive, leaving PublishAfter with zero callers. Removing it shrinks the publisher contract to a single verb and deletes the delayed-visibility machinery that existed only to serve it. ### What? Publisher loses PublishAfter; the mysql impl deletes its method, folds InsertDelayed back into Insert, drops FetchByOffset's nowMs parameter and its visible_after filter, removes the column from the queue_messages schema and the MoveToDLQ column list, and regenerates mocks. Tests covering delayed publish/fetch are deleted; the misnamed TestPublisher_PublishAfterClose (which tests Publish on a closed publisher) is renamed. READMEs drop the PublishAfter guidance — a consumer defers its own delivery with Hold/Postpone, and no caller needed cross-topic delayed publishing. Deployment note: the schema files here are applied by dev/test stacks; a live database needs `ALTER TABLE queue_messages DROP COLUMN visible_after` (old rows with a pending visible_after would become immediately visible on rollout, which is the correct degradation — those rows were minted by the pre-hold republish pattern and their consumers supersede/no-op them). ## Test Plan ✅ `make test` (83 targets). ✅ `bazel test //test/integration/extension/messagequeue/...` — full queue behavior including idempotent publish, DLQ, postpone barrier. ✅ `make fmt`, `make mocks`.
1 parent 93f5ace commit 30fb699

14 files changed

Lines changed: 23 additions & 240 deletions

File tree

platform/consumer/README.md

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,6 @@ Several mechanisms can delay work; they mean different things. Pick by what you'
133133
| "I'm still working — keep my lease" | `delivery.ExtendVisibilityTimeout(...)` | blocked behind the in-flight delivery |
134134
| "Done for now — wake this partition in N ms" | `delivery.Hold(N)` then `return nil` | paused behind the postponed message (barrier), redelivers first in order |
135135
| "Stop this controller/partition from outside" (tests, operators) | consumer gate (`platform/extension/consumergate`) | parked in flight until the gate opens |
136-
| "Defer *other* work" — a delayed message to another topic or key | `Publisher.PublishAfter` | not involved — it's a fresh publish |
137136

138137
Gate vs hold, since both pause a partition: the **gate** is an external, event-ended stop — someone stops the controller at the door, before `Process` ever sees the message. **Hold** is a controller-chosen, timer-ended wait — the controller saw the work and decided to come back later. Business logic never closes or opens gates; a controller that needs to back off uses hold.
139138

platform/extension/messagequeue/README.md

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,20 +13,12 @@ Publishes messages to topics.
1313
```go
1414
type Publisher interface {
1515
Publish(ctx context.Context, topic string, message entityqueue.Message) error
16-
PublishAfter(ctx context.Context, topic string, message entityqueue.Message, delayMs int64) error
1716
Close() error
1817
}
1918
```
2019

2120
(`entityqueue` is `github.com/uber/submitqueue/platform/base/messagequeue`.)
2221

23-
**`PublishAfter`** inserts a fresh message that becomes visible to subscribers only after `delayMs`. It is distinct from `Nack(requeueAfterMillis)` even though both can produce "next delivery happens at T+delay":
24-
25-
- `Nack` is "this delivery failed, try again" — it bumps `retry_count` and eventually trips DLQ.
26-
- `PublishAfter` is "postpone this work" — `retry_count` resets to 0, DLQ stays available for true failures.
27-
28-
For a consumer deferring its *own current delivery* ("check back in N ms"), prefer `Delivery.Postpone` (below) over ack-plus-`PublishAfter`: it needs no publisher, no fresh message id, and keeps the same log row. `PublishAfter` remains the tool for deferring *other* work — publishing a delayed message to a different topic or key. Use `Nack` for processing failures.
29-
3022
### Subscriber
3123
Consumes messages from topics with per-subscription configuration.
3224

platform/extension/messagequeue/mock/publisher_mock.go

Lines changed: 0 additions & 14 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

platform/extension/messagequeue/mysql/README.md

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,8 +112,6 @@ platform/extension/messagequeue/mysql/
112112
| `queue_partition_leases` | Partition lease coordination | `(consumer_group, topic, partition_key)` |
113113
| `queue_subscriber_heartbeats` | Active subscriber tracking | `(consumer_group, topic, subscriber_name)` |
114114

115-
`queue_messages` has a `visible_after BIGINT UNSIGNED NOT NULL DEFAULT 0` column that supports `Publisher.PublishAfter`: subscribers' `FetchByOffset` skips rows where `visible_after > now`. Default 0 means immediately visible, so existing rows continue to behave as before — the column is back-compatible.
116-
117115
`queue_delivery_state` has a `postponed BOOLEAN NOT NULL DEFAULT FALSE` column that supports `Delivery.Postpone`. `MarkPostponed` sets `invisible_until = now + delay`, resets `retry_count` to 0, and sets the flag. While the flag is set and the row is invisible, the poll loop treats the message as a **barrier** — it stops scanning the partition instead of skipping past it (nacked rows keep skip-and-continue semantics, so a failed message never halts its partition). On the next `MarkDelivered` the flag is consumed: the `retry_count` increment is skipped and the flag cleared, so a postponed redelivery restarts as attempt 1 and only consecutive real failures count toward `Retry.MaxAttempts`. Default FALSE keeps existing rows back-compatible.
118116

119117
See `schema/` for full SQL definitions. See the [RFC](../../../doc/rfc/sql-queue-rfc.md#database-schema) for field-level documentation.

platform/extension/messagequeue/mysql/message_store.go

Lines changed: 9 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -44,15 +44,7 @@ func newMessageStore(db *sql.DB, logger *zap.SugaredLogger, scope tally.Scope) m
4444
}
4545
}
4646

47-
// Insert inserts messages into the messages table with no visibility delay.
48-
// Equivalent to InsertDelayed with visibleAfterMs == 0.
49-
func (s *sqlmessageStore) Insert(ctx context.Context, topic string, messages []entityqueue.Message) error {
50-
return s.InsertDelayed(ctx, topic, messages, 0)
51-
}
52-
53-
// InsertDelayed inserts messages into the messages table, optionally deferring
54-
// delivery until visibleAfterMs (epoch milliseconds). 0 means immediately
55-
// visible; FetchByOffset skips rows where visible_after > now.
47+
// Insert inserts messages into the messages table.
5648
//
5749
// Publishes are idempotent on the (topic, partition_key, id) unique key: a
5850
// repeated publish for the same key is silently treated as success and does
@@ -61,7 +53,7 @@ func (s *sqlmessageStore) Insert(ctx context.Context, topic string, messages []e
6153
// idempotent publishes") and lets callers safely retry publishes (e.g. a
6254
// second Cancel RPC for the same request) without surfacing 1062 duplicate-key
6355
// errors.
64-
func (s *sqlmessageStore) InsertDelayed(ctx context.Context, topic string, messages []entityqueue.Message, visibleAfterMs int64) (retErr error) {
56+
func (s *sqlmessageStore) Insert(ctx context.Context, topic string, messages []entityqueue.Message) (retErr error) {
6557
op := metrics.Begin(s.scope, "insert", metrics.StorageLatencyBuckets, metrics.NewTag("topic", topic))
6658
defer func() { op.Complete(retErr) }()
6759

@@ -72,7 +64,6 @@ func (s *sqlmessageStore) InsertDelayed(ctx context.Context, topic string, messa
7264
s.logger.Debugw("inserting messages",
7365
logTopic, topic,
7466
"count", len(messages),
75-
"visible_after", visibleAfterMs,
7667
)
7768

7869
tx, err := s.db.BeginTx(ctx, nil)
@@ -84,8 +75,8 @@ func (s *sqlmessageStore) InsertDelayed(ctx context.Context, topic string, messa
8475
// ON DUPLICATE KEY UPDATE topic=topic is a no-op write that makes MySQL
8576
// swallow the unique-key violation without mutating the existing row.
8677
stmt, err := tx.PrepareContext(ctx, fmt.Sprintf(`
87-
INSERT INTO %s (topic, id, payload, metadata, partition_key, created_at, published_at, visible_after, failed_at, failure_count, last_error, original_topic)
88-
VALUES (?, ?, ?, ?, ?, ?, ?, ?, 0, 0, '', '')
78+
INSERT INTO %s (topic, id, payload, metadata, partition_key, created_at, published_at, failed_at, failure_count, last_error, original_topic)
79+
VALUES (?, ?, ?, ?, ?, ?, ?, 0, 0, '', '')
8980
ON DUPLICATE KEY UPDATE topic = topic
9081
`, MessagesTableName))
9182
if err != nil {
@@ -111,7 +102,6 @@ func (s *sqlmessageStore) InsertDelayed(ctx context.Context, topic string, messa
111102
msg.PartitionKey,
112103
now,
113104
msg.PublishedAt,
114-
visibleAfterMs,
115105
)
116106
if err != nil {
117107
return fmt.Errorf("insert message topic=%s message=%s partition=%s: %w", topic, msg.ID, msg.PartitionKey, err)
@@ -147,20 +137,18 @@ func (s *sqlmessageStore) Delete(ctx context.Context, topic string, partitionKey
147137
}
148138

149139
// FetchByOffset fetches messages with offset > currentOffset for a specific partition.
150-
// Rows whose visible_after > nowMs are skipped — those are deferred deliveries
151-
// (published via InsertDelayed) that should not yet be surfaced to subscribers.
152140
// Messages are fetched from the immutable log; no per-message mutation occurs.
153-
func (s *sqlmessageStore) FetchByOffset(ctx context.Context, topic string, partitionKey string, currentOffset int64, nowMs int64, limit int) (_ []messageRow, retErr error) {
141+
func (s *sqlmessageStore) FetchByOffset(ctx context.Context, topic string, partitionKey string, currentOffset int64, limit int) (_ []messageRow, retErr error) {
154142
op := metrics.Begin(s.scope, "fetch", metrics.StorageLatencyBuckets, metrics.NewTag("topic", topic))
155143
defer func() { op.Complete(retErr) }()
156144

157145
rows, err := s.db.QueryContext(ctx, fmt.Sprintf(`
158146
SELECT offset, id, payload, metadata, partition_key, published_at, failed_at, failure_count, last_error, original_topic
159147
FROM %s
160-
WHERE topic = ? AND partition_key = ? AND offset > ? AND visible_after <= ?
148+
WHERE topic = ? AND partition_key = ? AND offset > ?
161149
ORDER BY offset
162150
LIMIT ?
163-
`, MessagesTableName), topic, partitionKey, currentOffset, nowMs, limit)
151+
`, MessagesTableName), topic, partitionKey, currentOffset, limit)
164152
if err != nil {
165153
return nil, fmt.Errorf("query messages topic=%s partition=%s: %w", topic, partitionKey, err)
166154
}
@@ -267,12 +255,10 @@ func (s *sqlmessageStore) MoveToDLQ(ctx context.Context, topic string, partition
267255
}
268256

269257
// Insert into queue_messages table with DLQ topic name and DLQ-specific fields.
270-
// DLQ messages are always immediately visible (visible_after=0); any delay on
271-
// the original message has already been consumed by the time it failed.
272258
now := time.Now().UnixMilli()
273259
_, err = tx.ExecContext(ctx, fmt.Sprintf(`
274-
INSERT INTO %s (topic, id, payload, metadata, partition_key, created_at, published_at, visible_after, failed_at, failure_count, last_error, original_topic)
275-
VALUES (?, ?, ?, ?, ?, ?, ?, 0, ?, ?, ?, ?)
260+
INSERT INTO %s (topic, id, payload, metadata, partition_key, created_at, published_at, failed_at, failure_count, last_error, original_topic)
261+
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
276262
`, MessagesTableName), dlqTopic, messageID, payload, metadataJSON, fetchPartKey, createdAtMilli, publishedAtMilli, now, failureCount, lastError, topic)
277263

278264
if err != nil {

platform/extension/messagequeue/mysql/message_store_test.go

Lines changed: 2 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -141,68 +141,23 @@ func TestMessageStore_FetchByOffset(t *testing.T) {
141141
topic := "test_topic"
142142
partitionKey := "part1"
143143
currentOffset := int64(0)
144-
nowMs := time.Now().UnixMilli()
145144
limit := 10
146145

147146
// Mock query results (no transaction, simple SELECT)
148147
rows := sqlmock.NewRows([]string{"offset", "id", "payload", "metadata", "partition_key", "published_at", "failed_at", "failure_count", "last_error", "original_topic"}).
149148
AddRow(int64(1), "msg1", []byte("payload1"), []byte("{}"), "part1", time.Now().UnixMilli(), int64(0), 0, "", "")
150149

151150
mock.ExpectQuery("SELECT (.+) FROM queue_messages").
152-
WithArgs(topic, partitionKey, currentOffset, nowMs, limit).
151+
WithArgs(topic, partitionKey, currentOffset, limit).
153152
WillReturnRows(rows)
154153

155-
results, err := store.FetchByOffset(ctx, topic, partitionKey, currentOffset, nowMs, limit)
154+
results, err := store.FetchByOffset(ctx, topic, partitionKey, currentOffset, limit)
156155
require.NoError(t, err)
157156
require.Len(t, results, 1)
158157
require.Equal(t, "msg1", results[0].ID)
159158
require.NoError(t, mock.ExpectationsWereMet())
160159
}
161160

162-
func TestMessageStore_FetchByOffset_SkipsDelayed(t *testing.T) {
163-
db, mock, store := setupmessageStoreTest(t)
164-
defer db.Close()
165-
166-
ctx := context.Background()
167-
topic := "test_topic"
168-
partitionKey := "part1"
169-
currentOffset := int64(0)
170-
nowMs := int64(1000)
171-
limit := 10
172-
173-
// The SQL filter (visible_after <= nowMs) is applied by the DB; sqlmock just
174-
// verifies the parameter binding. An empty result row simulates the case
175-
// where the only message is still deferred.
176-
mock.ExpectQuery("SELECT (.+) FROM queue_messages").
177-
WithArgs(topic, partitionKey, currentOffset, nowMs, limit).
178-
WillReturnRows(sqlmock.NewRows([]string{"offset", "id", "payload", "metadata", "partition_key", "published_at", "failed_at", "failure_count", "last_error", "original_topic"}))
179-
180-
results, err := store.FetchByOffset(ctx, topic, partitionKey, currentOffset, nowMs, limit)
181-
require.NoError(t, err)
182-
require.Empty(t, results)
183-
require.NoError(t, mock.ExpectationsWereMet())
184-
}
185-
186-
func TestMessageStore_InsertDelayed(t *testing.T) {
187-
db, mock, store := setupmessageStoreTest(t)
188-
defer db.Close()
189-
190-
ctx := context.Background()
191-
visibleAfter := time.Now().UnixMilli() + 5000
192-
msg := entityqueue.Message{ID: "msg-delayed", Payload: []byte("p"), PartitionKey: "part1", PublishedAt: time.Now().UnixMilli()}
193-
194-
mock.ExpectBegin()
195-
mock.ExpectPrepare("INSERT INTO queue_messages")
196-
mock.ExpectExec("INSERT INTO queue_messages").
197-
WithArgs("test_topic", msg.ID, msg.Payload, []byte(nil), msg.PartitionKey, sqlmock.AnyArg(), msg.PublishedAt, visibleAfter).
198-
WillReturnResult(sqlmock.NewResult(1, 1))
199-
mock.ExpectCommit()
200-
201-
err := store.InsertDelayed(ctx, "test_topic", []entityqueue.Message{msg}, visibleAfter)
202-
require.NoError(t, err)
203-
require.NoError(t, mock.ExpectationsWereMet())
204-
}
205-
206161
func TestMessageStore_MoveToDLQ(t *testing.T) {
207162
db, mock, store := setupmessageStoreTest(t)
208163
defer db.Close()

platform/extension/messagequeue/mysql/mock_stores.go

Lines changed: 4 additions & 18 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

platform/extension/messagequeue/mysql/publisher.go

Lines changed: 0 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import (
1818
"context"
1919
"fmt"
2020
"sync"
21-
"time"
2221

2322
"github.com/uber-go/tally"
2423
"go.uber.org/zap"
@@ -67,36 +66,6 @@ func (p *publisher) Publish(ctx context.Context, topic string, message entityque
6766
return nil
6867
}
6968

70-
// PublishAfter sends a message that becomes visible to subscribers only
71-
// after delayMs from now. The message is inserted with visible_after =
72-
// now + delayMs; FetchByOffset skips it until that timestamp.
73-
// delayMs <= 0 is equivalent to Publish.
74-
func (p *publisher) PublishAfter(ctx context.Context, topic string, message entityqueue.Message, delayMs int64) (retErr error) {
75-
op := metrics.Begin(p.scope, "publish_after", metrics.StorageLatencyBuckets, metrics.NewTag("topic", topic))
76-
defer func() { op.Complete(retErr) }()
77-
78-
p.mu.RLock()
79-
closed := p.closed
80-
p.mu.RUnlock()
81-
82-
if closed {
83-
return ErrPublisherClosed
84-
}
85-
86-
var visibleAfter int64
87-
if delayMs > 0 {
88-
visibleAfter = time.Now().UnixMilli() + delayMs
89-
}
90-
91-
if err := p.messageStore.InsertDelayed(ctx, topic, []entityqueue.Message{message}, visibleAfter); err != nil {
92-
return fmt.Errorf("publish_after message store insert error: %w", err)
93-
}
94-
95-
p.logger.Debugw("published delayed message", logTopic, topic, logMessageID, message.ID, "delay_ms", delayMs)
96-
97-
return nil
98-
}
99-
10069
// Close gracefully shuts down the publisher
10170
func (p *publisher) Close() error {
10271
p.mu.Lock()

platform/extension/messagequeue/mysql/publisher_test.go

Lines changed: 1 addition & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ func TestPublisher_Publish(t *testing.T) {
140140
}
141141
}
142142

143-
func TestPublisher_PublishAfterClose(t *testing.T) {
143+
func TestPublisher_PublishOnClosedPublisher(t *testing.T) {
144144
ctrl := gomock.NewController(t)
145145
defer ctrl.Finish()
146146

@@ -160,68 +160,6 @@ func TestPublisher_PublishAfterClose(t *testing.T) {
160160
require.True(t, errors.Is(err, ErrPublisherClosed))
161161
}
162162

163-
func TestPublisher_PublishAfter(t *testing.T) {
164-
tests := []struct {
165-
name string
166-
delayMs int64
167-
wantVisibleArg gomock.Matcher
168-
}{
169-
{
170-
name: "positive delay binds future visible_after",
171-
delayMs: 5000,
172-
// Exact timestamp depends on wall clock; assert it's > 0.
173-
wantVisibleArg: gomock.Cond(func(v any) bool {
174-
ts, ok := v.(int64)
175-
return ok && ts > 0
176-
}),
177-
},
178-
{
179-
name: "zero delay binds visible_after=0",
180-
delayMs: 0,
181-
wantVisibleArg: gomock.Eq(int64(0)),
182-
},
183-
{
184-
name: "negative delay clamps to 0",
185-
delayMs: -100,
186-
wantVisibleArg: gomock.Eq(int64(0)),
187-
},
188-
}
189-
190-
for _, tt := range tests {
191-
t.Run(tt.name, func(t *testing.T) {
192-
ctrl := gomock.NewController(t)
193-
defer ctrl.Finish()
194-
195-
mockStore := NewMockmessageStore(ctrl)
196-
mockStore.EXPECT().
197-
InsertDelayed(gomock.Any(), "test_topic", gomock.Any(), tt.wantVisibleArg).
198-
Return(nil).
199-
Times(1)
200-
201-
pub := setupPublisherTest(t, mockStore)
202-
203-
msg := entityqueue.NewMessage("msg-delayed", []byte("p"), "part1", nil)
204-
err := pub.PublishAfter(context.Background(), "test_topic", msg, tt.delayMs)
205-
require.NoError(t, err)
206-
})
207-
}
208-
}
209-
210-
func TestPublisher_PublishAfterClosed(t *testing.T) {
211-
ctrl := gomock.NewController(t)
212-
defer ctrl.Finish()
213-
214-
mockStore := NewMockmessageStore(ctrl)
215-
pub := setupPublisherTest(t, mockStore)
216-
217-
require.NoError(t, pub.Close())
218-
219-
msg := entityqueue.NewMessage("msg1", []byte("p"), "part1", nil)
220-
err := pub.PublishAfter(context.Background(), "test_topic", msg, 1000)
221-
require.Error(t, err)
222-
require.True(t, errors.Is(err, ErrPublisherClosed))
223-
}
224-
225163
func TestPublisher_Close(t *testing.T) {
226164
ctrl := gomock.NewController(t)
227165
defer ctrl.Finish()

platform/extension/messagequeue/mysql/schema/queue_messages.sql

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,6 @@ CREATE TABLE IF NOT EXISTS queue_messages (
2525
created_at BIGINT UNSIGNED NOT NULL,
2626
published_at BIGINT UNSIGNED NOT NULL,
2727

28-
-- visible_after defers delivery: subscribers skip rows where visible_after > now.
29-
-- 0 (the default) means immediately visible. Set by Publisher.PublishAfter
30-
-- to schedule a fresh message for delivery at a future time without
31-
-- consuming a delivery_state retry slot (used e.g. by the orchestrator's
32-
-- buildstatus polling consumer to space out Status calls).
33-
visible_after BIGINT UNSIGNED NOT NULL DEFAULT 0,
34-
3528
-- DLQ-specific fields (0/"" for normal messages, populated for DLQ messages)
3629
failed_at BIGINT UNSIGNED NOT NULL,
3730
-- failure_count stores how many times the message failed on the ORIGINAL topic before moving to DLQ

0 commit comments

Comments
 (0)