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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ref(cardinality): Use a Lua script and in-memory cache for the cardinality limiter #2849
ref(cardinality): Use a Lua script and in-memory cache for the cardinality limiter #2849
Changes from 2 commits
20d0799
2df679e
1157c5c
2782500
a26c9f6
1cd6c57
37369c2
5db88ff
ac109fc
ac35d4b
806eec9
23f7250
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should batch exclusively on the Relay side. That would simplify this script, and we need some sort of batching there anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like batching in the lua script, makes the relay side easier to read and we have a guarantee that this never fails in lua (one less error to handle and invariant).
The lua implementation would get slightly easier, but we would still need a slicing function like
batches
(to sliceoffset
andmax
), which now is just a minimal overhead to thebatches
function (just a change of the max value and the offset).So overall imo we don't gain much by moving it to relay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense!
Even if we keep batching on the Lua side, don't we also need batching on the Rust side? Not sure if we would run into size limits or network timeouts otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how Redis and the network behaves here, in my local tests it didn't make a difference but there is also no latency and Redis does not do anything else (e.g. rate limiting etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we batch and use a pipeline, I feel like that would make no difference then?