DiceDB / dice

DiceDB is a redis-compliant, reactive, scalable, highly-available, unified cache optimized for modern hardware.
https://dicedb.io/
Other
6.86k stars 1.09k forks source link

#1016 Migrated `INCR` commands #1141

Closed pg30 closed 1 month ago

pg30 commented 1 month ago

Closes #1016

apoorvyadav1111 commented 1 month ago

Hi, Thank you for making these changes.

Please add integration tests in resp, http and websocket folder. Additionally, please audit and update documentation for all the migrated commands. If this PR is still in progress, you could mark it as Draft, which will allow you to post update, ask questions, get pre-reviewed but not get Changes requested.

Edit: lint check is failing, please check.

Please reach out to me or anyone on the discord community if you have any questions.

Thanks Apoorv

pg30 commented 1 month ago

Have added a new error to make the INCRBYFLOAT command consistent with redis.

    ErrInvalidFloat               = errors.New("ERR value is not a valid float")                                         // Signals that a value provided is not in a valid float format.

Have also added documentation for INCRBY and INCRBYFLOAT commands as well.

pg30 commented 1 month ago

@apoorvyadav1111 can you pre-review this if i need to do any changes. Thanks

apoorvyadav1111 commented 1 month ago

Hi @pg30 , I have checked the changes as per the checklist. I see that we are still having tests in async folder. We can migrate them to resp folder, (please use any existing test file in resp folder as reference). Additionally, we need to add tests in websocket folder too.

I will look at the changes thoroughly once it checks off everything and is ready for review.

Thanks

AshwinKul28 commented 1 month ago

hi @pg30 you can use ErrGeneral for the error if it's not getting used at multiple evals, no need to create a separate error for this usecase. Thanks

pg30 commented 1 month ago

Thanks for the feedback @apoorvyadav1111 @AshwinKul28 Will make the changes accordingly

pg30 commented 1 month ago

@AshwinKul28 can you review this pls

pg30 commented 1 month ago

@AshwinKul28 Http and Websockets integration tests in the PR involving big integers like math.MaxInt64 and math.MinInt64 are functionally working but not logically correct due to #1163 . Can create a separate issue to clean up those once we fix it.

AshwinKul28 commented 1 month ago

Hey @pg30 Thanks a lot for this commendable effort. Changes looks good. Please verify all the docs changes once whether they are aligned with the ideal document - https://github.com/DiceDB/dice/blob/master/docs/sample_command_docs.md

You can include best practices, and additional notes if required in each of these docs.

Otherwise, the changes look great. Once docs changes are confirmed will merge the PR.

pg30 commented 1 month ago

@AshwinKul28 Thanks for reviewing the changes. I have added alternatives section for the INCR and DECR commands. Rest of the docs looks good to me. Feel free to merge the PR