DiceDB
Migrate SADD, SREM, SCARD, SMEMBERS command to store_eval
Description Migrates the eval functions for SADD, SREM, SMEMBERS, SCOUNT to the new eval function type (https://github.com/DiceDB/dice/issues/1020)
Checklist
- [x] Migrated the evalXXX function with the latest definition
- [x] Update or add unit tests for the new implementation.
- [x] All unit tests pass successfully.
- [x] Ensure all integration tests pass successfully.
- [x] Ensure integration tests are added for the migrated command on multi-threaded resp server.
- [x] Move Integration tests for the respective commands under the RESP integration tests directory from Async directory
- [ ] Please validate that the documentation for the respective commands is up to date. If not then consider adding them.
HI @sahoss changes look good. 🚀 left a minor comment.
Please pull the latest from the master to resolve the linter issue. Thanks
HI @sahoss Thanks for the changes, LGTM! Please add migration for 'SMEMBERS', 'SCARD' also in the same PR.
@AshwinKul28 added the remaining commands. however, i still see lint errors locally and i have merged latest master into my branch. its not from my changes. ok to ignore?
Hi @sahoss thanks a lot for the changes. Can you please add unit tests for each eval? It's not present already but you can add new unit tests for each command in the eval_test.go Also please rebase it with master.
@sahoss lets ensure that the corresponding integration tests are migrated as well. cc @AshwinKul28
@AshwinKul28 / @soumya-codes - sure. it will take me until this weekend potentially to make those changes. just a heads up
Just FYI, we need to add the tests for the migrated commands under integration_tests/commands/resp
I have added a new item to checklist in the PR description. Please tick the same once the integration tests are added. Please let me or Ashwin know if you have any doubts/concern.
It looks like there are a few conflicts @sahoss, could you please rebase on the latest master?
@sahoss Thanks a lot for the commendable efforts. I have left very minor suggestions, please have a look.
I know it's much more to ask, but please go through the docs repository and check the docs for the commands you are migrating. Apologies for the back-and-forth, but we want to make these commands production-ready at once.
Here's sample documentation to follow
SADD, SCARD commands documentation is almost ready but SREM and SMEMEMBERS need some refresh.
@sahoss Thanks a lot for the commendable efforts. I have left very minor suggestions, please have a look.
I know it's much more to ask, but please go through the docs repository and check the docs for the commands you are migrating. Apologies for the back-and-forth, but we want to make these commands production-ready at once.
Here's sample documentation to follow
SADD, SCARD commands documentation is almost ready but SREM and SMEMEMBERS need some refresh.
Thank you for your feedback/reviews @AshwinKul28 Really appreciate it.
just to make sure i understood your comment on refreshing the documentation for srem/smembers - i see that current documentation https://github.com/DiceDB/docs/blob/master/src/content/docs/commands/SMEMBERS.md, https://github.com/DiceDB/docs/blob/master/src/content/docs/commands/SREM.md covers the operations quite well(imo). its at the same level of coverage as SADD/SCARD. is the ask to make the formatting similar to the sample documentation you have shared?
@sahoss Thanks a lot for the commendable efforts. I have left very minor suggestions, please have a look. I know it's much more to ask, but please go through the docs repository and check the docs for the commands you are migrating. Apologies for the back-and-forth, but we want to make these commands production-ready at once. Here's sample documentation to follow SADD, SCARD commands documentation is almost ready but SREM and SMEMEMBERS need some refresh.
Thank you for your feedback/reviews @AshwinKul28 Really appreciate it.
just to make sure i understood your comment on refreshing the documentation for srem/smembers - i see that current documentation https://github.com/DiceDB/docs/blob/master/src/content/docs/commands/SMEMBERS.md, https://github.com/DiceDB/docs/blob/master/src/content/docs/commands/SREM.md covers the operations quite well(imo). its at the same level of coverage as SADD/SCARD. is the ask to make the formatting similar to the sample documentation you have shared?
yes
HI @sahoss Hope you're doing well, Do you still have any blockers on this? Let me know and we can resolve them quickly and try to merge the PR. thanks a lot for all the efforts.
@AshwinKul28 i had a question for you here https://github.com/DiceDB/dice/pull/1068#discussion_r1809361475.
i am working in parallel to refresh the documentation.
Thanks @sahoss for the documentation refresh. I have left a single comment on the docs PR! Otherwise, is everything related to the migration sorted, or are HTTP tests failing?
@AshwinKul28 everything should be passing now. i have addressed the documentation PR comment
@sahoss Please address above requested changes by Apoorv, then we are good to merge. thanks
@apoorvyadav1111 still 1 conflict in the store eval file :(
i have fixed the conflict.
thanks @apoorvyadav1111 @AshwinKul28 for your detailed reviews. appreciate your time and effort on this pr.
Hi @sahoss , apologies for commiting and reverting the deque test change. We recently migrated to this package and would not want to revert the efforts and reopen another issue to migrate again. I fixed the issue, it was a call to old assert package, which could have been reverted as part of recent merge.
@AshwinKul28 Thanks for your review. I am merging this as the changes after your review are minimal and we don't want another conflict. Thank you both for making this change into main.
