feat(clickhouse sink): add retry_strategy option - #26221
Open
jamesdangercarpenter wants to merge 4 commits into
Open
feat(clickhouse sink): add retry_strategy option#26221jamesdangercarpenter wants to merge 4 commits into
jamesdangercarpenter wants to merge 4 commits into
Conversation
The clickhouse sink hardcoded ClickhouseRetryLogic::default(), so there was
no way to change which HTTP responses it treats as retriable. The http sink
already exposes RetryStrategy; this brings the clickhouse sink in line.
The default strategy classes any non-5xx other than 429/408 as
non-retriable, which includes the 404 ClickHouse returns for an unknown
table. That interacts badly with end-to-end acknowledgements on the kafka
source. A non-retriable response drops the batch and finalizes it as
Rejected; the source only calls store_offset for Delivered batches, so a
rejected batch does not advance the offset by itself. But Kafka commits a
single watermark per partition, so the next batch that *does* succeed
stores an offset past every rejected batch behind it.
Observed in a dev migration: six shadow consumers started against a
ClickHouse service whose schema had not been created yet. Five of them
replayed their full retained backlog in about three minutes, dropped all of
it against the missing tables, and then committed past the lot the moment
the schema appeared and the first batch succeeded. Consumer lag went to
zero, which reads as a healthy catch-up. Roughly 1.4M events were lost
across four topics with no error surviving anywhere except the drop counter.
Retrying instead means the batch never resolves, OrderedFinalizer stalls
behind it, and the watermark cannot move:
retry_strategy:
type: custom
status_codes: [401, 403, 404, 408, 429]
The 500 handling is deliberately untouched. ClickHouse reports malformed
data as a 500 with a `Code: 117` or `Code: 53` body, and those stay
non-retriable under every strategy — retrying a poison pill would wedge
every batch queued behind it. Tests cover both directions plus the carve-out.
Note that this does not make loss impossible: a batch that legitimately
cannot be retried still advances the watermark past itself when the next one
succeeds. component_discarded_events_total is the only signal for that, and
it deserves an alert rather than a dashboard panel.
github-actions
Bot
added
domain: sinks
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eedd51d482
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Author
|
I have read the CLA Document and I hereby sign the CLA |
Two gaps in the previous commit, both reported in review. should_retry_response hardcoded RetryAction::Retry for any 500 whose body was not Code: 117 or Code: 53, so a strategy of `none`, or a `custom` list without 500, was ignored for exactly the status ClickHouse uses most. With unlimited retries by default that stalls ingest on a response the operator explicitly asked not to retry. Only the malformed-data exception overrides the strategy now; every other response, 500s included, is delegated. ClickhouseRetryLogic also did not implement is_retriable_timeout, so it inherited the RetryLogic default of true instead of forwarding to the inner HttpRetryLogic, which returns false for `none`. That contradicted the documented promise of `none` not to retry timeouts, and risks duplicate inserts after an ambiguous timeout. It forwards now. Tests cover both: a 500 stays non-retriable under `none` and under a custom list that excludes it, and is_retriable_timeout tracks the strategy.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0715f8abe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Now that ordinary 500s follow the configured strategy, the documented custom example ([401, 403, 404, 408, 429]) would disable the default 5xx retries and drop batches on transient server errors — the opposite of its loss-avoidance intent. Recommend `type: all` instead, and note that a custom list must include the server errors the default would have retried.
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
1 participant
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.
Summary
The
clickhousesink hardcodedClickhouseRetryLogic::default(), so there was no way to change which HTTP responses it treats as retriable. Thehttpsink already exposesRetryStrategy; this brings theclickhousesink in line by adding aretry_strategyconfig option (default unchanged).Why this matters: a response classified as non-retriable makes the sink drop the batch and finalize it as
Rejected. With end-to-end acknowledgements from thekafkasource, that is not inert - the source only stores offsets forDeliveredbatches, but Kafka commits a single watermark per partition, so the next batch that does succeed stores an offset past every rejected batch behind it. The default strategy classes any non-5xx other than 429/408 as non-retriable, which includes the 404 ClickHouse returns for an unknown table.We hit this during a migration: six consumers started against a ClickHouse service whose schema had not yet been created. Five replayed their full retained backlog in about three minutes, dropped all of it against the missing tables, then committed past the lot the moment the schema appeared and the first batch succeeded. Consumer lag went to zero — which reads as a healthy catch-up — and roughly 1.4M events were lost across four topics with no signal anywhere except the drop counter.
With this option, deployments that must not lose events can retry such responses instead, so the batch never resolves and the watermark cannot advance past it:
The one thing the strategy does not govern is malformed data: ClickHouse reports it as a 500 with a
Code: 117orCode: 53body, and those stay non-retriable under every strategy - retrying a poison pill would stall ingest indefinitely. Every other response, other 500s included, follows the configured strategy, sononeand acustomlist that excludes 500 are honoured there too.Changes:
src/sinks/util/http.rs: addHttpRetryLogic::new(RetryStrategy)for sinks that surfaceretry_strategyin their own configuration.src/sinks/clickhouse/{config,service}.rs: add theretry_strategyoption and thread it intoClickhouseRetryLogic, keeping the malformed-data (Code 117/53) exception on top of whatever strategy is configured.ClickhouseRetryLogicforwardsis_retriable_timeoutto the innerHttpRetryLogicsononealso suppresses timeout retries.References
Vector configuration
How did you test this PR?
noneand acustomlist without 500 leave a 500 non-retriable,is_retriable_timeouttracks the strategy, and malformed-data 500s (Code: 117/Code: 53) stay non-retriable under every strategy (cargo test --lib sinks::clickhouse, 46 passed).cargo check,cargo fmt --check, clippy clean on the changed files.make generate-component-docs.Is this a breaking change?
Does this PR include user facing changes?
no-changeloglabel to this PR.