Repository navigation
feat(topology): add opt-in reload sink drain limit - #26834
thomasqueirozb wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 379217b1ca
ℹ️ 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".
| /// events. Other unread and in-flight events are lost. If not set, Vector waits until the | ||
| /// sinks finish. |
There was a problem hiding this comment.
Describe background sink draining accurately
When the option is unset, a reload does not wait for every removed sink: shutdown_diff explicitly drops the task handle for sinks outside wait_for_sinks, allowing them to drain in the background while the reload continues (src/topology/running.rs:718-724). This help text, also repeated in the changelog and generated CLI reference, therefore misstates the existing behavior; describe the unset case as having no cancellation deadline, with only resource-conflicting or buffer-reusing sinks awaited.
Useful? React with 👍 / 👎.
| let deadline = self | ||
| .reload_sink_drain_duration | ||
| .map(|limit| Instant::now() + limit); |
There was a problem hiding this comment.
Reject drain limits that overflow Instant
When --reload-sink-drain-limit-secs or its environment variable is set to a sufficiently large nonzero u64 (for example 18446744073709551615), clap accepts it as NonZeroU64, but the first sink-changing reload panics here because adding that duration to Instant::now() overflows. Validate the CLI value or use checked_add so an accepted setting cannot crash Vector during reload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deadline cancellation can leave spawned requests holding a disk-buffer lock. For affected sinks, changing disk-buffer settings can then fail both replacement and rollback, shutting Vector down instead of completing the reload.
🤖 Bits Code Review · Commit 379217b
| () = reload_deadline => { | ||
| warn!("Sink did not finish before the reload deadline. Cancelling in-flight work."); | ||
| // Drop only the sink future, retaining unread buffered events for its replacement. | ||
| Ok(()) |
There was a problem hiding this comment.
Cancel spawned requests before reporting sink shutdown
With the drain limit enabled, changing disk-buffer settings for an influxdb_logs or gcp_pubsub sink while a request remains pending beyond the deadline can terminate Vector. PartitionBatchSink spawns request tasks that survive dropping sink.run. Their event finalizers keep the disk ledger’s buffer.lock held, so both the replacement and rollback fail to open the same buffer, triggering FailedToRestore and shutdown. Cancel and await spawned requests before reporting the sink stopped; this requires changes beyond this cancellation branch.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
Summary
Add
--reload-sink-drain-limit-secs(VECTOR_RELOAD_SINK_DRAIN_LIMIT_SECS) so that a sink that does not stop cannot block a configuration reload. All removed and changed sinks share one deadline. The flag is unset by default, and--graceful-shutdown-limit-secsdoes not apply to reloads.Delivered, so the source acknowledges them upstreamThe data loss is opt-in and documented in the flag help. A follow-up PR will mark in-flight events as
Erroredwhen a sink is cancelled, so sources with acknowledgements retry them instead of acknowledging lost data.References
Depends on: #26832
Related: #26793
Vector configuration
How did you test this PR?
topology::running::tests: the deadline is shared and unread events move to the replacement; without the flag, the shutdown grace period does not cancel a stalled sink.vector --reload-sink-drain-limit-secs 5with the config above, changed the codec tojson, and sent SIGHUP. The reload finished and output switched to JSON.--reload-sink-drain-limit-secs 0is rejected.Does this PR include user facing changes?
no-changeloglabel to this PR.