Repository navigation
Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bound the retry path's attempt lookup by the failed attempt's time, so each lookup reads a few granules instead of the whole attempts table.
Problem
Before it publishes a retry, the retry scheduler looks up the latest attempt for the event and destination:
On ClickHouse the table's sort key starts with
attempt_time, and the query has no time condition, so only the bloom skip indexes apply. The tenant and destination blooms pass almost every granule (few distinct values); theevent_idbloom keeps about 2.5 % of all granules as false positives. Cost grows linearly with history: with 8 M rows over 30 days each lookup reads ~13 MiB (192k rows, ~1,500 marks) and uses ~0.3 s of ClickHouse CPU, even for an event with a single attempt. The payload column is not the cost (lazy materialization reads it only for the returned rows); the key columns of the false-positive granules are. On 3 vCPU that caps retries at ~10 lookups/s, so a burst of failed deliveries saturates ClickHouse for minutes.Idea
The retry task knows when the attempt that scheduled it happened. Carrying that time in the task lets the lookup start there: the latest attempt can't be older than the attempt that failed.
RetryTaskgetsEventTimeandAttemptTime(json:",omitzero"), set when the retry is scheduled from the failed attempt.TimeFilter.GTE = AttemptTime - 1son the existingListAttemptRequest. All three log store drivers (ClickHouse, Postgres, memory) already support it; no driver code changes. The 1 s covers storage precision (the stored attempt time is the same value the task carries; ClickHouse keeps ms).AttemptTime(queued before the upgrade) run today's unbounded query.There is no upper bound: attempts newer than the carried one (later automatic or manual retries) are still found.
Benchmarked on the same 8 M-row table (read, rows, marks: one lookup, warm caches; CPU and throughput: 50 concurrent lookups, ClickHouse at its 3 vCPU limit):
Postgres already finds the row with an index seek on
event_id(idx_attempts_event_time, ~0.1 ms at 2 M rows); the bound addstime >= ...to the same index condition and keeps one code path.Behavior changes
LOG_BATCH_THRESHOLD_SECONDS), the lookup now returns nothing and the task goes through the normal exec-error backoff until the write lands. Before, it found the attempt before that one, if any, and continued with a stale attempt number. fix: requeue retries whose attempt is not persisted yet without counting receives #1154 treats this case as "not ready" without counting it as a failed receive.Results
Local reproduction, 18,000 events in 60 s, destination failing for the first 20 s (setup below):
mainThe remaining pool timeouts come from the scheduler starting one goroutine per received retry against a 10-connection pool (capped in #1153). The 21 undelivered events stopped after their first retry because of a delete race in the scheduler, fixed in #1151; builds that include that fix (#1153, #1154) delivered all 18,000.
How the numbers were measured
Local reproduction: ClickHouse 25.8 limited to 3 vCPU with 8 M rows in the attempts table (30 days, ~90 KB average payload), 3 Outpost instances (all services in each, 2 CPUs, 2 GiB), Redis-compatible store for the retry queue, NATS as the internal queue. Retry schedule
1, 5, 300, 1800, ...seconds,LOG_BATCH_SIZE=50,LOG_BATCH_THRESHOLD_SECONDS=10,RETRY_VISIBILITY_TIMEOUT_SECONDS=30. Workload: 18,000 events (12 KB payload) in 60 s to one webhook destination that returns 5xx for the first 20 s, then 200 (about 6,000 failed first attempts, so about 6,000 retries due within ~20 s). Lookup cost fromsystem.query_log; ClickHouse CPU from the container's cgroup. One run per build; three runs of themainbaseline agreed within 3 % on pool timeouts and delivered events (lookup p50 duration 3.2-3.6 s).Tests
internal/deliverymq/retrytask_test.go: JSON compatibility (old payload decodes with zero times and runs unbounded; new payload round-trips; zero times omitted).Not in this PR
🤖 Generated with Claude Code