dice icon indicating copy to clipboard operation
dice copied to clipboard
verified publisher icon DiceDB

Command Migration: ('HINCRBY', 'HINCRBYFLOAT', 'HRANDFIELD')

Open sashpawar11 opened this issue 1 year ago • 18 comments

This PR migrates HINCRBY, HINCRBYFLOAT and HRANDFIELD to make it protocol agnostic. Closes #1023

Migration 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] Add http, websocket tests for the commands
  • [x] Move Integration tests for the respective commands under the RESP integration tests directory from Async directory
  • [x] Please validate that the documentation for the respective commands is up to date. If not then consider adding them.

Note:

  • Http, Resp and Websocket tests to be added as well since they are missing for the addressed commands under this issue.
  • Docs for these commands have been added

sashpawar11 avatar Oct 13 '24 10:10 sashpawar11

Hi @AshwinKul28 update 10/15 - unit tests have been migrated, please have a look and provide feedback! The unit tests have passed succesfully.

Will continue to work on migrating the integration tests. Thankyou.

sashpawar11 avatar Oct 15 '24 05:10 sashpawar11

@AshwinKul28 - the HRANDFIELD command depends on selectRandomFields() for returning the random field. Should we move it to hash.go which feels more relevant as it has other utility methods around hashmaps? Currently, selectRandomFields()has been migrated to store_eval.go from eval.go under this PR

sashpawar11 avatar Oct 15 '24 05:10 sashpawar11

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

soumya-codes avatar Oct 15 '24 13:10 soumya-codes

Hi @AshwinKul28 - for the last task, I'm not sure how to proceed on it. Are these commands optimized for multi-threaded execution yet?

The unit tests and integration tests have passed in the latest build

sashpawar11 avatar Oct 15 '24 16:10 sashpawar11

@sashpawar11 please refer to existing integration tests under integration_tests/commands/resp for the commands that are already migrated. We will need to do the same these commands as well.

soumya-codes avatar Oct 15 '24 16:10 soumya-codes

@soumya-codes Awesome, thanks!! Will have a look at it and migrate these commands. Thankyou

sashpawar11 avatar Oct 15 '24 16:10 sashpawar11

Update 10/16 - added migrated resp tests for the commands. All the tasks have been completed and build passed, however, the http and websocket integration tests are not present for these commands. Added an additional task in the description for the same, will complete it by today/tomorrow and update here once ready for review.

cc @AshwinKul28 @soumya-codes

sashpawar11 avatar Oct 16 '24 05:10 sashpawar11

Update 10/18 - Added all the missing HTTP integrations tests for the command. Will try to complete the web-socket tests asap and mark this PR for review. cc @AshwinKul28 @soumya-codes

sashpawar11 avatar Oct 17 '24 18:10 sashpawar11

Hi @AshwinKul28 @soumya-codes @lucifercr07 - all the commands have been migrated and their respective tests have been added. Resolved multiple merge conflicts along the way.

The PR Is ready for review.

The documentation for the commands are missing and I'll try to add it via a separate PR as this one is already a bit large. Thankyou.

sashpawar11 avatar Oct 18 '24 05:10 sashpawar11

@AshwinKul28 - the HRANDFIELD command depends on selectRandomFields() for returning the random field. Should we move it to hash.go which feels more relevant as it has other utility methods around hashmaps? Currently, selectRandomFields()has been migrated to store_eval.go from eval.go under this PR

Also, please advise on this. cc @AshwinKul28 @soumya-codes @lucifercr07

sashpawar11 avatar Oct 18 '24 05:10 sashpawar11

Can we also check the doc related to 'HINCRBY', 'HINCRBYFLOAT', 'HRANDFIELD' commands also once, if there's any discrepancy?

Minor comments added.

Hey @lucifercr07 - there no docs present for these commands at the moment. Will be adding them in a seperate pr as I complete working on it.

sashpawar11 avatar Oct 18 '24 14:10 sashpawar11

Hi @AshwinKul28 @soumya-codes @lucifercr07 - all the tasks have been completed. Please review the changes!

Latest changes:

  • Documentation for the commands have been added as well.
  • selectrandomfields() has been moved over to hmap.go

sashpawar11 avatar Oct 19 '24 17:10 sashpawar11

Hey @sashpawar11 this is a commendable effort. Everything looks great to me. Few cosmetic changes are required. (I know this is a little back and forth, apologies for that. But we want to make it a one-time effort, and some ideal PRs to merge)

AshwinKul28 avatar Oct 19 '24 20:10 AshwinKul28

Thanks @AshwinKul28 ! And yes I agree, lets complete all the changes to make this a one-time effort

sashpawar11 avatar Oct 20 '24 03:10 sashpawar11

Thanks @sashpawar11 Please pull the latest from the master, I see ZRANK command implementation and documentation got removed in your PR. Please rebase with the master. Everything else looks great. Thanks again!

AshwinKul28 avatar Oct 20 '24 11:10 AshwinKul28

Thanks @sashpawar11 Please pull the latest from the master, I see ZRANK command implementation and documentation got removed in your PR. Please rebase with the master. Everything else looks great. Thanks again!

Fixed , ZRANK is back. Ready for merge. Thankyou.

sashpawar11 avatar Oct 20 '24 12:10 sashpawar11

Ahh, another PR got merged. 🗡️ Please resolve conflicts quickly @sashpawar11 and I will merge. Thanks

AshwinKul28 avatar Oct 20 '24 13:10 AshwinKul28

Ahh, another PR got merged. 🗡️ Please resolve conflicts quickly @sashpawar11 and I will merge. Thanks

Hey, will try to resolve soon

sashpawar11 avatar Oct 20 '24 13:10 sashpawar11

Conflicts resolved. Ready for merge. Thankyou

sashpawar11 avatar Oct 20 '24 15:10 sashpawar11