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.

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

#1019 Migrated LPUSH, RPUSH, LPOP, RPOP, LLEN #1181

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

Aditya-Chowdhary
Copy link
Contributor

@Aditya-Chowdhary Aditya-Chowdhary commented Oct 22, 2024

fixes #1019

Edit: Now ready for review
Currently draft PR as updating integration tests is remaining. Remaining tasks are completed.

For migration commands, please consider this a template on how to tackle the issue. I have divided the tasks into smaller goals and made an individual commit.

Condition Return Value
Migrating eval to store eval
Modify unit test with new response format
Migrate integration tests from async to resp
Add/update integration in websocket
Add/update integration in HTTP
Ensure all integration test pass
Update/Add docs for the commands
Lint checks
  • 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 (Resp/http/websocket)
  • Move Integration tests for the respective commands under the RESP integration tests directory from the Async directory
  • Please validate that the documentation for the respective commands is up to date. If not then consider adding them.

@Aditya-Chowdhary Aditya-Chowdhary marked this pull request as ready for review October 22, 2024 14:56
@AshwinKul28 AshwinKul28 self-requested a review October 22, 2024 21:26
@AshwinKul28 AshwinKul28 added the migration -- command Migrates current eval to a refactored eval for all protocols functionality label Oct 22, 2024
Copy link
Contributor

@lucifercr07 lucifercr07 left a comment

Choose a reason for hiding this comment

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

@Aditya-Chowdhary can we follow sample doc template and update for these commands, accordingly it'd be helpful.

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 @Aditya-Chowdhary Thanks a lot for these changes. I have requested changes mostly in return responses, where while sending error we shouldn't send Result as clientio.NIL, instead it should be just nil.

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
internal/eval/store_eval.go Show resolved Hide resolved
internal/eval/store_eval.go Outdated Show resolved Hide resolved
internal/eval/store_eval.go Outdated Show resolved Hide resolved
@Aditya-Chowdhary
Copy link
Contributor Author

@AshwinKul28 I have made the requested changes. Several of the ones you commented on did not return an error, so I have left those as clientio.NIL for now, please let me know if I should change those ones as well.

https://github.com/DiceDB/dice/pull/1181/files#r1818149611
https://github.com/DiceDB/dice/pull/1181/files#r1818150264
https://github.com/DiceDB/dice/pull/1181/files#r1818150407

@AshwinKul28
Copy link
Contributor

HI @Aditya-Chowdhary thanks again for the commendable efforts. The changes look great.

Please rebase your branch with the master and I have added a checklist in the PR description, go through it and complete those tasks. (Most of them already you have taken care :D)

For the documentation of LPUSH, RPUSH, RPOP commands we need a refresher based on this sample doc such as example descriptions, notes and best practices if required.

@Aditya-Chowdhary
Copy link
Contributor Author

Aditya-Chowdhary commented Nov 6, 2024

Apologies for the delay in this, it's been a busy couple of days, I'll rebase the changes tomorrow(7/11).

@Aditya-Chowdhary
Copy link
Contributor Author

@JyotinderSingh @AshwinKul28 I have completed the rebase and checked the checklist.

@JyotinderSingh
Copy link
Collaborator

There is an integration test failure, could you please take a look?

@Aditya-Chowdhary Aditya-Chowdhary force-pushed the feat/migrations branch 2 times, most recently from bf5da19 to fd583ee Compare November 11, 2024 20:39
@Aditya-Chowdhary
Copy link
Contributor Author

Aditya-Chowdhary commented Nov 11, 2024

@JyotinderSingh Have fixed all merge conflicts + test failures
Edit: Grammar

Copy link
Contributor

@apoorvyadav1111 apoorvyadav1111 left a comment

Choose a reason for hiding this comment

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

Just a small change and we are good to merge

config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
@apoorvyadav1111 apoorvyadav1111 merged commit 9c35e90 into DiceDB:master Nov 12, 2024
1 check passed
@apoorvyadav1111
Copy link
Contributor

Thanks for taking up this issue and closing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migration -- command Migrates current eval to a refactored eval for all protocols functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command Migration: ('LPUSH', 'RPUSH', 'LPOP', 'RPOP', 'LLEN')
5 participants