perf(cache): should delete multiple keys at once to reduce operations in Redis cache #6170
Conversation
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
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to f090886 in 20 seconds
More details
- Looked at
56lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. pkg/query-service/cache/redis/redis_test.go:85
- Draft comment:
TheRemovefunction now usesBulkRemove, which expects multiple keys. Update the test to reflect this change by expectingExpectDelwith a single-element slice. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment seems to be suggesting a change that has already been made in the diff. The test has been updated to useExpectDelwith multiple keys, which matches the functionality ofBulkRemove. Therefore, the comment is redundant and not necessary.
I might be missing some context about theRemovefunction's previous implementation, but the current test implementation seems correct based on the diff.
The diff clearly shows that the test has been updated to useExpectDelwith multiple keys, which is consistent with theBulkRemovefunction's behavior.
The comment is unnecessary because the test already reflects the change to useExpectDelwith multiple keys. It should be deleted.
2. pkg/query-service/cache/redis/redis.go:69
- Draft comment:
Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. This comment applies to theRemovefunction and any other similar instances. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_VnROTeADvXjcaWaM
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
git-hulk
force-pushed
the
improve/cache-del-multi-keys
branch
from
f090886 to
cd4aa70
Compare
git-hulk
changed the title
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.
Summary
Currently, it will send multiple commands to Redis if there are more than
one key in BulkRemove. We can improve by sending them at once to reduce operations.
Important
Optimize Redis cache operations by modifying
RemoveandBulkRemoveto delete multiple keys in a single command, reducing Redis operations.Removeinredis.gonow callsBulkRemoveto delete a single key, optimizing the operation.BulkRemoveinredis.gomodified to delete multiple keys in a single Redis command.errors.Isfor error comparison inRetrieveinredis.go.TestBulkRemoveinredis_test.goupdated to expect a singleDelcommand for multiple keys.This description was created by
for f090886. It will automatically update as commits are pushed.