This repository has been archived by the owner on Jun 21, 2024. It is now read-only.
capture: add overflow_enabled option #43
Merged
Conversation
This file contains 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
tomasfarias
reviewed
May 27, 2024
Comment on lines
+67
to
+70
| let partition = partition.clone(); | ||
| tokio::spawn(async move { | ||
| partition.clean_state().await; | ||
| }); |
There was a problem hiding this comment.
Huh, wondering if we should Arc also the OverflowLimiter's forced_keys HashSet, since that's being cloned here it could be expensive if it's too large.
There was a problem hiding this comment.
Anyways, seems like that was there from before, not a blocker on this PR.
There was a problem hiding this comment.
Agreed. forced_keys is empty though, supported as a last resort if the volume is so big that plugin-server cannot forward to overflow fast enough. Cloning it as long as it has a zero or a couple of entries might be cheaper than the Arc.
Should probably be superseeded by a redis zset instead.
tomasfarias
approved these changes
May 27, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
We had disabled capture-led overflow on cloud for 6 weeks now (by disabling it on python, and setting very high values on rust). This overflow accounting is using a significant amount of memory and I want to double-check that the cleanups are not contributing to latency.
OVERFLOW_ENABLEDenvvar, default to false as that's what we want for hobby/local + the current behaviour on cloud (very high values)KafkaSinknow takes aOption<OverflowLimiter>. Could have extracted a trait, but checking for the Option is probably cheaper than calling a method that always returnstrue. Plus we might just want to rip it out altogether and only keepoverflow_forced_keys?