DiceDB
Add support for `GET` in `SET` command
We already have the SET command implemented. Now we need to add support for GET option similar to the GET option in Redis. Please refer to the following commit in Redis to understand the implementation specifics - source.
Write unit and integration tests for the command referring to the tests written in the Redis codebase 7.2.5. For integration tests, you can refer to the tests folder. Note: they have used TCL for the test suite, and we need to port that to our way of writing integration tests using the relevant helper methods. Please refer to our tests directory.
For the command, benchmark the code and measure the time taken and memory allocs using benchmem and try to keep them to the bare minimum.
@arpitbbhayani , could you please guide that do we need more flags for the GET command, like here in redis:
The EX, PX etc... Am I right?
@arpitbbhayani , could you please guide that do we need more flags for the
GETcommand, like here in redis:The
EX,PXetc... Am I right?
You only need to implement the GET option for this issue
Ohh! Ok, now I got that. earlier mykey was = "world";
So, when the user runs - SET mykey "hello" GET, it should return world (since, this was the previous key), am I right @JyotinderSingh ?
Hi @JyotinderSingh , I apologize, actually, this issue, was inactive for 2 days 🙏 I'm working on this issue, actually, I was trying to get familiar with the codebase. I've started working on this, and I'll create a draft PR by tomorrow 👍
Just wanted to give an update that this issue is not inactive...
@JyotinderSingh , ig there are some issues with the PR for this issue. I'll just close it and create a new one. Apologies for inconvenience 🙏
Hello @yaten2302,
There has been no activity on this issue for the past 5 days. It would be awesome if you keep posting updates to this issue so that we know you are actively working on it.
We are really eager to close this issue at the earliest, hence if we continue to see the inactivity, we will have to reassign the issue to someone else. We are doing this to ensure that the project maintains its momentum and others are not blocked on this work.
Just drop a comment with the current status of the work or share any issues you are facing. We can always chip in to help you out.
Thanks again.
Hey @arpitbbhayani , yes, I'm working on this issue. Ig you also mentioned on the PR linked to this issue. Actually, due to some issues, I was inactive for a few days(I mentioned on Discord, too).
Also, for this issue, the NX option with the GET option is failing. I'll add some commits today for this. I'll surely ask if I've any doubt.
And, also, apologies for the delay🙏
Hi @yaten2302, Please feel free to share the issues you are facing with this implementation here, we would be happy to help. Since this has been open for a long time now, let's try to close it soon.
Thanks for working on this!
@shardul08 its inactive for longtime can i take this issue?
Let's wait for @yaten2302's response. You could later ask one of the maintainers to assign you the issue.
Hi @JyotinderSingh , I noticed that this issue has been inactive for some time. I'd be happy to pick it up and work on it. Please let me know if it's okay.
Hi @JyotinderSingh , I noticed that this issue has been inactive for some time. I'd be happy to pick it up and work on it. Please let me know if it's okay.
Assigned to you @sanjaydev02
#322 has almost all required changes, please refer to my last comments on that PR to see how to fix a few issues. Please ensure you add a comprehensive test suite.
Hi @JyotinderSingh , I noticed that this issue has been inactive for some time. I'd be happy to pick it up and work on it. Please let me know if it's okay.
Assigned to you @sanjaydev02
Thanks @JyotinderSingh for assigning this to me! I'll start working on it right away. I'll reach out if I encounter any issues. Looking forward to resolving this!
Hi @JyotinderSingh, I have some questions about my code. Can you help me when you have time? Thanks!"
Hi @JyotinderSingh, I have some questions about my code. Can you help me when you have time? Thanks!"
Please list the questions here.
@JyotinderSingh when i'm adding case GET inside the switch case for checking if GET option is passed or not it throws error like its undefined.
@JyotinderSingh when i'm adding case GET inside the switch case for checking if GET option is passed or not it throws error like its undefined.
Please raise a draft PR, we can discuss there. It's hard to understand the issue with so little detail.
Hi @JyotinderSingh , I've created a draft PR (link to PR). Could you please check it and provide your feedback? Thank you!
Hello @sanjaydev02,
There has been no activity on this issue for the past 5 days. It would be awesome if you keep posting updates to this issue so that we know you are actively working on it.
We are really eager to close this issue at the earliest, hence if we continue to see the inactivity, we will have to reassign the issue to someone else. We are doing this to ensure that the project maintains its momentum and others are not blocked on this work.
Just drop a comment with the current status of the work or share any issues you are facing. We can always chip in to help you out.
Thanks again.
@arpitbbhayani I’ve created a draft PR and would really appreciate your feedback so I can move forward. I’m still a beginner and any guidance from your side would be very helpful.
Hi @JyotinderSingh ,
I'm still working on this issue and wanted to give you a quick update. Apologies for the delay, I realize it's a simple one, and I'll have it resolved ASAP.
Thanks for your patience!
Hi @JyotinderSingh ,
I'm still working on this issue and wanted to give you a quick update. Apologies for the delay, I realize it's a simple one, and I'll have it resolved ASAP.
Thanks for your patience!
Awesome, eagerly waiting for the PR!
Hi @sanjaydev02 , Hope you are well. I am eager to know more about any progress you have made on this issue. As you know, we are looking forward to closing this issue, please reach to me or anyone else on the discord community if you need help.
@apoorvyadav1111 can i be assigned this issue? would love to work on it.
Hi @KanniShashankh, Apologies, It was originally assigned to @sanjaydev02 and was unassigned as he closed his original PR. He recently communicated that he created a new PR with migrated logic.