Migrate GETEX and GETDEL commands

[Ehijoe]

Description

Migrates the eval functions for GETEX and GEDEL to the new eval function type independent of protocol (fixes #1012 ).

Checklist

• Migrated the evalXXX function with the latest definition
• Update or add unit tests for the new implementation.
• All unit tests pass successfully.
• Ensure all integration tests pass successfully.
• Ensure integration tests are added for the migrated command on multi-threaded resp server.
• 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.

Notes/Issues

• evalGETEX currently calls evalGET to simplify getting raw responses. Should the implementations be separated and if so should that logic be factored out since it's pretty much the same?
• When testing manually, I noticed that after an error the next command returns (nil) no matter what the next command is although I haven't investigated why that happens
• Integration tests expect errors with the commands in lowercase. Should errors be reported like that? e.g.:

expected ERR wrong number of arguments for 'GETDEL' command

rather than:

expected ERR wrong number of arguments for 'getdel' command

[AshwinKul28]

HI @Ehijoe Thanks for the changes.

Notes/Issues

• evalGETEX currently calls evalGET to simplify getting raw responses. Should the implementations be separated and if so should that logic be factored out since it's pretty much the same?
• When testing manually, I noticed that after an error the next command returns (nil) no matter what the next command is although I haven't investigated why that happens
• Integration tests expect errors with the commands in lowercase. Should errors be reported like that? e.g.:

expected ERR wrong number of arguments for 'GETDEL' command

rather than:

expected ERR wrong number of arguments for 'getdel' command

1. Let's keep implementations of eval separated as it is. The meaning I understood about factoring out is, again modularising the code and using the same at both places. But again logic might get combined. Lets keep it separated as you have done for now. Later we can make use of decomposeCommand function from the worker to decompose GETEX to GET Ex.

2. I did not see this in the current master

3. Good point about lowercase, I will push a PR before you to make it lowercase so you don't need to make changes
   (Changes are pushed, please pull latest from master)

[AshwinKul28]

HI @Ehijoe Hope you are doing well. are there any updates on this PR? If there're any blockers please let us know here or via discord. Thanks

[Ehijoe]

Hi @AshwinKul28. I'm really sorry for the delay. I've merged the changes from master and made some fixes.

[JyotinderSingh]

Hi @AshwinKul28. I'm really sorry for the delay. I've merged the changes from master and made some fixes.

Not sure if we can see those changes, can you make sure you've pushed them?

[Ehijoe]

I added a constant for PERSIST to fix the formatting. It should pass the checks now.

[AshwinKul28]

HI @Ehijoe Thanks for the changes, left one minor comment. Please ellaborate. Otherwise things looks awesome.

[AshwinKul28]

Thanks @Ehijoe for the changes.

[soumya-codes]

@Ehijoe lets ensure that the corresponding integration tests are migrated as well. cc @AshwinKul28

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.

[Ehijoe]

Okay @soumya-codes I'll get on that now. I just have to duplicate the same test cases from async and http right?

[JyotinderSingh]

Okay @soumya-codes I'll get on that now. I just have to duplicate the same test cases from async and http right?

Yes

[AshwinKul28]

@Ehijoe please rebase your PR with master. let us know if there's any blocker left. Thanks

[Ehijoe]

Hi @AshwinKul28. I've added the integration tests and rebased off master. Should be good to go now right?

[soumya-codes]

@Ehijoe thanks a ton for raising the PR! The progress look great.

We are adding an additional task for ensuring the documentation is up-to-date and proper for the commands.
Apologies for this additional task.
The reason we are doing this is to ensure we are production ready once the migration of commands is ready.
I have added an item to the checklist regarding the same.
Please reach out to me or @AshwinKul28 if you see any concerns.

[Ehijoe]

Hi @soumya-codes. I've deleted the async integration tests. About the documentation, it seems to be accurate (the web docs in the docs directory at least) but is there anything in particular you'd like me to check?

[soumya-codes]

@Ehijoe thanks for migrating the tests. We are just one step away.

@apoorvyadav1111 has added a document-template/sample-document for the command documentation.

Please refer to the following files:
https://github.com/DiceDB/dice/blob/master/docs/command_docs_template.md
https://github.com/DiceDB/dice/blob/master/docs/sample_command_docs.md

We need to align our documentation around this.

[soumya-codes]

@Ehijoe the PR looks in great shape.
Left couple of comments.
Please let me know wdyt.

[soumya-codes]

@Ehijoe please let us know if you have any concerns? Except for the one fix and documentation alignment, everything looks great.

I understand the last minute addition of documentation is cumbersome, the only reason it was added to PR is because DiceDB is aiming to be Prod Ready by November first week. We have added this to all command migration PRs. This is important for better adoption of DiceDB.

[JyotinderSingh]

@Ehijoe Could you please address the last few comments? The PR is almost ready for review.

[AshwinKul28]

Hi @Ehijoe thanks a lot for the changes. Please resolve the conflicts.
Also, please take care of documentation for both the commands, which needs to be aligned with the Sample doc

Need to add entires for these commands under /worker/cmd_meta.go file.

Also, make sure to run test suite and check if any changes required in the HTTP and websocket integrations tests along with the unit tests.

[Ehijoe]

Hi @Ehijoe thanks a lot for the changes. Please resolve the conflicts. Also, please take care of documentation for both the commands, which needs to be aligned with the Sample doc

Need to add entires for these commands under /worker/cmd_meta.go file.

Also, make sure to run test suite and check if any changes required in the HTTP and websocket integrations tests along with the unit tests.

Hi @AshwinKul28 the docs seem to be up to date. You're referring to https://github.com/Ehijoe/dice/blob/issue-1012/docs/src/content/docs/commands/GETEX.md and https://github.com/Ehijoe/dice/blob/issue-1012/docs/src/content/docs/commands/GETDEL.md right?

[AshwinKul28]

HI @Ehijoe yes, the docs are almost ready, but I can see the example n: prefix to all examples, which we need to remove and also need to add a one-liner explanation about all examples. In the GETDEL example I see explanation after example that needs to ve moved as a sub-title to the example title, these minor details to make it similar to the sample doc.

[AshwinKul28]

LGTM. Thanks for the commendable efforts @Ehijoe .