Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added support for GET in SET command #246

Closed
wants to merge 7 commits into from
Closed

Added support for GET in SET command #246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f773350
feat: added support for GET in SET command
yaten2302 Aug 6, 2024
d1766b2
Merge branch 'DiceDB:master' into master
yaten2302 Aug 6, 2024
9045f21
Merge branch 'DiceDB:master' into master
yaten2302 Aug 6, 2024
2d26c4d
Merge branch 'DiceDB:master' into master
yaten2302 Aug 6, 2024
77e926e
added test cases for GET command option in SET
yaten2302 Aug 6, 2024
9fc1361
removed Put from case GET
yaten2302 Aug 7, 2024
15a4584
Merge branch 'DiceDB:master' into master
yaten2302 Aug 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions core/eval.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,15 @@ func evalSET(args []string) []byte {
if obj != nil {
return RESP_NIL
}
case "GET":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are adding GET, we cannot now blindly return from XX and NX.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm not able to get it completely, could you please elaborate more on this

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @soumya-codes , I was just now running the tests with the XX option with the GET one. The test is failing. I think do we need to change the complete evalSET() func? I'm thinking of an approach for this, will update soon about it here 👍

obj := Get(key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just call evalGET() method before the value has been updated and return the o/p of evalGET() as is. wdyt?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so, like, I've to use evalGET() function instead of Get(), am I right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@soumya-codes , I've a doubt, that since by using the evalGET function, we need to pass an array

image

So, first, we've to push the key to an array and then pass it to the evalGET func, which will increase the fetching time, so can we just let it be Get() only? because in other cases also, the Get() function is only used!
What say?


if obj == nil {
return RESP_NIL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it is better to just update the response here. Let us invoke PUT at just one place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. We don't have to call PUT in 2 places. Passing GET will just change the return value of the SET command.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, actually, previously I did by using Put in the end only, but there was a review from someone which mentioned that the value won't get updated if I didn't add Put at 2 places, this comment.

NVM, I'll make the changes 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need to add Put in two places but you will need a variable for a response and then you can return that in the end if it was of subtype GET instead of RESP_OK. If you just return it from here only then PUT never gets called and SET command doesn't execute. That is what I meant

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! srivastav-yash, apologies for the miscommunication. Now I have got your point. But, actually, before doing that, I'm first trying to fix the issue with XX and NX options. If we're giving both xx and get, then the tests are failing.

}

// Return the RESP encoded value
return Encode(obj.Value, false)
yaten2302 marked this conversation as resolved.
Show resolved Hide resolved
default:
return Encode(errors.New("ERR syntax error"), false)
}
Expand Down
5 changes: 5 additions & 0 deletions tests/set_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,11 @@ func TestSetWithOptions(t *testing.T) {
commands: []string{"SET k v XX EX 1", "GET k", "SLEEP 2", "GET k", "SET k v XX EX 1", "GET k"},
expected: []interface{}{"(nil)", "(nil)", "OK", "(nil)", "(nil)", "(nil)"},
},
{
yaten2302 marked this conversation as resolved.
Show resolved Hide resolved
name: "GET option",
commands: []string{"SET k v", "SET k v2 GET"},
expected: []interface{}{"OK", "v"},
},
}

for _, tc := range testCases {
Expand Down