-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
#1031: Bloom Filter - Migrated bf.add
, bf.reserve
, bf.exists
& bf.info
to store_eval
#1117
#1031: Bloom Filter - Migrated bf.add
, bf.reserve
, bf.exists
& bf.info
to store_eval
#1117
Conversation
bf.add
, bf.reserve
, bf.exists
& bf.info
to store_eval
7494344
to
e4a24d9
Compare
afb541c
to
66dd05e
Compare
@@ -63,7 +64,7 @@ func TestObjectCommand(t *testing.T) { | |||
}, | |||
{ | |||
name: "Object Encoding check for bf", | |||
commands: []string{"BFADD bloomkey value1", "BFADD bloomkey value2", "OBJECT ENCODING bloomkey"}, |
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.
We can remove bf tests from async. I see since you have already implemented it in resp we can remove it from here. Or have you just kept it to check that the current functionality is intact?
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 have kept it here so that it can be migrated when Object commands are migrated.
Thanks for this PR @apoorvyadav1111 . Fantastic effort. Everything looks good and we can now reuse evalError and evalResponse functions in all store evals. We may not ask others to do the changes again if other PRs are already in the last stage, we will do it by ourselves once all commands are migrated. Also, I see your commit links are broken in the PR description for now, once it's added to master I believe it should work. Thanks again. I just have one comment, post resolution of that feel free to merge. |
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.
LGTM
Hi @AshwinKul28 , thanks for the review. It must be due to force-push, for the safer side, I will update the commits once the branch gets merged and will not be modified. |
Hi @AshwinKul28 , Apologies I merged it before looking at the new commits, I can definitely raise a new PR right now and add those comments. I have updated the commit hashes for now. |
Fixes: #1031 #1110
For migration commands, please consider this a template on how to tackle the issue. I have divided the tasks into smaller goals and made an individual commit.
Checklist