Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#1022 Command Migration: ('HEXISTS', 'HKEYS', 'HVALS') #1087

Merged
merged 37 commits into from
Oct 28, 2024
Merged

#1022 Command Migration: ('HEXISTS', 'HKEYS', 'HVALS') #1087

merged 37 commits into from
Oct 28, 2024

Conversation

tarungka
Copy link
Contributor

@tarungka tarungka commented Oct 14, 2024 •
edited
Loading

Migrates the eval functions for HEXISTS, HKEYS, HVALS to the new eval function type independent of protocol.
Closes #1022

Here is my check list:

  • 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.
  • Test that each protocol (RESP, HTTP, WebSocket) works correctly after migration.
  • 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.

@tarungka tarungka mentioned this pull request Oct 14, 2024
Closed
4 tasks
@tarungka tarungka marked this pull request as draft October 14, 2024 03:26
@AshwinKul28 AshwinKul28 added the migration -- command Migrates current eval to a refactored eval for all protocols functionality label Oct 15, 2024
AshwinKul28
AshwinKul28 requested changes Oct 15, 2024
View reviewed changes
Copy link
Contributor

@AshwinKul28 AshwinKul28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HI @tarungka thanks for the PR. please check the review comments and consider adding RESP integration tests as well.

internal/eval/store_eval.go Outdated Show resolved Hide resolved
internal/eval/store_eval.go Outdated Show resolved Hide resolved
internal/eval/store_eval.go Outdated Show resolved Hide resolved
internal/eval/store_eval.go Outdated Show resolved Hide resolved
internal/eval/store_eval.go Outdated Show resolved Hide resolved
internal/eval/store_eval.go Outdated Show resolved Hide resolved
internal/eval/store_eval.go Outdated Show resolved Hide resolved
test.log Outdated Show resolved Hide resolved
@soumya-codes
Copy link
Contributor

soumya-codes commented Oct 15, 2024 •
edited
Loading

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

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.

@tarungka
Copy link
Contributor Author

@AshwinKul28 I have updated the eval functions, will add the test cases shortly.

@soumya-codes I think you might have missed adding the item to the checklist, I'll check it off once added. Thanks.

@tarungka tarungka marked this pull request as ready for review October 17, 2024 15:58
@tarungka
Copy link
Contributor Author

@AshwinKul28 I've added the integration test please review the PR, I will fix the conflicts if the changes look good.

@AshwinKul28
Copy link
Contributor

HI @tarungka I see that most of the integration tests are still under the async directory, please take an effort to move them under the resp directory. Thanks

@soumya-codes
Copy link
Contributor

@tarungka please let us know if you have any concerns or facing any issues.

@tarungka
Copy link
Contributor Author

Hi @apoorvyadav1111, i have fixed almost all the test cases.

There is some new code recently merged, causing tests that return empty array responses to fail. I will look into that shortly.

@apoorvyadav1111
Copy link
Contributor

apoorvyadav1111 commented Oct 24, 2024 •
edited
Loading

Hi @tarungka, Sure, thanks. Please run unit, integration tests and linter after rebase with latest in local. If those pass, tag me and I will review and run workflow here.

Edit: you can also request review from all the reviewers

apoorvyadav1111
apoorvyadav1111 requested changes Oct 24, 2024
View reviewed changes
integration_tests/commands/async/hkeys_test.go Outdated Show resolved Hide resolved
integration_tests/commands/async/hvals_test.go Outdated Show resolved Hide resolved
integration_tests/commands/http/hexists_test.go Outdated Show resolved Hide resolved
integration_tests/commands/http/hexists_test.go Show resolved Hide resolved
integration_tests/commands/http/hkeys_test.go Outdated Show resolved Hide resolved
integration_tests/commands/resp/hkeys_test.go Show resolved Hide resolved
integration_tests/commands/resp/hkeys_test.go Show resolved Hide resolved
integration_tests/commands/resp/hvals_test.go Show resolved Hide resolved
test.log Outdated Show resolved Hide resolved
integration_tests/commands/resp/set_test.go Outdated Show resolved Hide resolved
@apoorvyadav1111
Copy link
Contributor

Hi, I have left some comments, in addition to them, please use the "github.com/stretchr/testify/assert" in all test files related to the commands under the scope of this PR. Also all the tests need a final cleanup. Currently, we delete keys before test, therefore the last testcase have keys remaining which might affect the further tests.

@tarungka
Copy link
Contributor Author

@AshwinKul28 @apoorvyadav1111 @lucifercr07 I have made the necessary changes, please review.

@AshwinKul28
Copy link
Contributor

Hello @tarungka , hope you are doing well. All the codebase looks fantastic. I see there's still no documentation available for these commands. I know it's little back and forth but we want to close it once-for-all. Can you please consider this template and try adding documentation pages for each of these three commands? Again, thanks a lot for the commendable efforts.

@tarungka
Copy link
Contributor Author

Hi @AshwinKul28 sure, I will add these command docs. Do I create a markdown file for each of the three commands under docs/?

@AshwinKul28
Copy link
Contributor

AshwinKul28 commented Oct 27, 2024 •
edited
Loading

Hi @AshwinKul28 sure, I will add these command docs. Do I create a markdown file for each of the three commands under docs/?

Yes, please. thanks @tarungka

@tarungka
Copy link
Contributor Author

@AshwinKul28 @apoorvyadav1111 added the docs, please review.

@apoorvyadav1111
Copy link
Contributor

Amazing work! Thanks for finishing this up, will merge once the checks are successful.

@apoorvyadav1111 apoorvyadav1111 changed the title Command Migration: ('HEXISTS', 'HKEYS', 'HVALS') #1022 Command Migration: ('HEXISTS', 'HKEYS', 'HVALS') Oct 28, 2024
@apoorvyadav1111 apoorvyadav1111 merged commit 2083901 into DiceDB:master Oct 28, 2024
2 checks passed
rushabhk04 pushed a commit to rushabhk04/dice that referenced this pull request Nov 1, 2024
@tarungka @rushabhk04
DiceDB#1022 Command Migration: ('HEXISTS', 'HKEYS', 'HVALS') (DiceDB#…
615e77d 
…1087)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soumya-codes soumya-codes soumya-codes left review comments

@apoorvyadav1111 apoorvyadav1111 apoorvyadav1111 approved these changes

@AshwinKul28 AshwinKul28 Awaiting requested review from AshwinKul28

Assignees
No one assigned
Labels
migration -- command Migrates current eval to a refactored eval for all protocols functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Command Migration: ('HEXISTS', 'HKEYS', 'HVALS')
4 participants
@tarungka @soumya-codes @AshwinKul28 @apoorvyadav1111