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

Add LMove command #577

Merged
merged 6 commits into from
May 19, 2022
Merged

Add LMove command #577

merged 6 commits into from
May 19, 2022

Conversation

torwig
Copy link
Contributor

@torwig torwig commented May 13, 2022

  • LMove is able to operate on a single list if the source and destination are the same
  • In the case of two lists LMove acquires two locks and performs Pop and Push atomically

I wrote only unit tests because I'm not currently familiar with TCL.

- LMove is able to operate on a single list if the source and destination are the same
- In case of two lists LMove acquires two locks and performs Pop and Push atomically
@git-hulk
Copy link
Member

git-hulk commented May 13, 2022

Thanks @torwig.

No worry, I can add test cases before merging.

@ShooterIT ShooterIT added feature type new feature release notes labels May 13, 2022
@torwig
Copy link
Contributor Author

torwig commented May 13, 2022

@git-hulk Could you please explain a little, why did the job [lint-build-test-on-ubuntu (ubuntu-18.04)] fail? There are some issues with TCL tests. However, I didn't add anything to them, except increase the number of commands.

@git-hulk
Copy link
Member

@git-hulk Could you please explain a little, why did the job [lint-build-test-on-ubuntu (ubuntu-18.04)] fail? There are some issues with TCL tests. However, I didn't add anything to them, except increase the number of commands.

@torwig Yes, the failure was NOT related with this PR. The root cause some test cases would fail occasionally when the CI environment become slowly. We have expected to find out those and fix them.

@git-hulk
Copy link
Member

@torwig I submitted the test case PR to your repository, you can have a look and merge into your branch if it's ok. torwig#1

@torwig
Copy link
Contributor Author

torwig commented May 14, 2022

@git-hulk Thank you. I merged your commit into my branch.

@git-hulk
Copy link
Member

OK, thanks. Will have a look soon.

src/lock_manager.cc Outdated Show resolved Hide resolved
@git-hulk
Copy link
Member

Good unit test coverage, thanks @torwig 👍

git-hulk
git-hulk previously approved these changes May 16, 2022
Copy link
Member

@ShooterIT ShooterIT left a comment

Choose a reason for hiding this comment

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

cool, i ever wanted to add multi lock, but no action, thanks

src/lock_manager.h Show resolved Hide resolved
Changelog Outdated Show resolved Hide resolved
@ShooterIT
Copy link
Member

one problem i need to confirm is that this command may not adapt slot migration, we should handle it in batch_extractor.cc

cc @ChrisZMF

@git-hulk
Copy link
Member

one problem i need to confirm is that this command may not adapt slot migration, we should handle it in batch_extractor.cc

cc @ChrisZMF

@ShooterIT Do you think is it a good idea to separate commands between Kvrocks and Kvrocks2Redis? Or we can do this in the next PR.

@ShooterIT
Copy link
Member

@ShooterIT Do you think is it a good idea to separate commands between Kvrocks and Kvrocks2Redis? Or we can do this in the next PR.

fine, let's complete in the next PR.

Copy link
Member

@ShooterIT ShooterIT left a comment

Choose a reason for hiding this comment

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

Thanks for impressive work @torwig

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature type new feature release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants