You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/05/13 09:28:42 UTC
[GitHub] [incubator-kvrocks] torwig opened a new pull request, #577: Add LMove command
torwig opened a new pull request, #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577
- 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#discussion_r873179049
##########
src/lock_manager.cc:
##########
@@ -51,3 +51,7 @@ void LockManager::Lock(const rocksdb::Slice &key) {
void LockManager::UnLock(const rocksdb::Slice &key) {
mutex_pool_[hash(key)]->unlock();
}
+
+void LockManager::LockTwo(const rocksdb::Slice &first_key, const rocksdb::Slice &second_key) {
+ std::lock(*mutex_pool_[hash(first_key)], *mutex_pool_[hash(second_key)]);
Review Comment:
@torwig You can have a look at: https://github.com/torwig/kvrocks/pull/2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#discussion_r873452426
##########
Changelog:
##########
@@ -1,5 +1,8 @@
# Version 999.999.999
+New features
+ - Add LMove command
+
Review Comment:
@ShooterIT I removed the record from Changelog. Sorry, didn't see that your suggestion can be committed just by clicking one button.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#discussion_r872983043
##########
src/lock_manager.cc:
##########
@@ -51,3 +51,7 @@ void LockManager::Lock(const rocksdb::Slice &key) {
void LockManager::UnLock(const rocksdb::Slice &key) {
mutex_pool_[hash(key)]->unlock();
}
+
+void LockManager::LockTwo(const rocksdb::Slice &first_key, const rocksdb::Slice &second_key) {
+ std::lock(*mutex_pool_[hash(first_key)], *mutex_pool_[hash(second_key)]);
Review Comment:
It will be deadlock when them have the same hash value. BTW, we can extend to multi locks instead of only two, we can reuse it later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
torwig commented on PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#issuecomment-1126697561
@git-hulk Thank you. I merged your commit into my branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#issuecomment-1125847625
Thanks @torwig.
No worry, I can add test cases after merging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#issuecomment-1126625076
@torwig I submitted the test case PR to your repository, you can have a look and merge into your branch if it's ok. https://github.com/torwig/kvrocks/pull/1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#discussion_r873640031
##########
src/lock_manager.h:
##########
@@ -55,3 +58,25 @@ class LockGuard {
LockManager *lock_mgr_ = nullptr;
rocksdb::Slice key_;
};
+
+class MultiLockGuard {
+ public:
+ explicit MultiLockGuard(LockManager *lock_mgr, const std::vector<std::string> &keys):
+ lock_mgr_(lock_mgr) {
+ locks_ = lock_mgr_->MultiGet(keys);
+ for (const auto &iter : locks_) {
+ iter->lock();
+ }
+ }
+
+ ~MultiLockGuard() {
+ // Lock with order `A B C` and unlock should be `C B A`
Review Comment:
Yes, for the `MultiLockGuary`, it's NOT different, but I think it's a good habit the lock and unlock with the reverse order.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#discussion_r872996633
##########
src/lock_manager.cc:
##########
@@ -51,3 +51,7 @@ void LockManager::Lock(const rocksdb::Slice &key) {
void LockManager::UnLock(const rocksdb::Slice &key) {
mutex_pool_[hash(key)]->unlock();
}
+
+void LockManager::LockTwo(const rocksdb::Slice &first_key, const rocksdb::Slice &second_key) {
+ std::lock(*mutex_pool_[hash(first_key)], *mutex_pool_[hash(second_key)]);
Review Comment:
Aha, we are using `crc(key)%buckets` to calculate the hash value, so different keys can't promise the value always be different.
> Is it possible that more than 2 locks should be locked?
To be honest, I'm not sure. What I think is that `MultiLocks` contains 2 locks and more generic.
> multi-version should deal with templates :)
All is ok to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#issuecomment-1126743384
Good unit test coverage, thanks @torwig 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT commented on pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#issuecomment-1128483342
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#discussion_r873158083
##########
src/lock_manager.cc:
##########
@@ -51,3 +51,7 @@ void LockManager::Lock(const rocksdb::Slice &key) {
void LockManager::UnLock(const rocksdb::Slice &key) {
mutex_pool_[hash(key)]->unlock();
}
+
+void LockManager::LockTwo(const rocksdb::Slice &first_key, const rocksdb::Slice &second_key) {
+ std::lock(*mutex_pool_[hash(first_key)], *mutex_pool_[hash(second_key)]);
Review Comment:
My bad. They are two points:
1. Lock two keys without checking the hash value may cause deadlock
2. `MultiLockGuard` may be better than `TwoLockGuard` since it can be used in other scenarios
The current implementation also makes sense to me. Except we should check whether the hash value is same or not, only lock once if yes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
torwig commented on PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#issuecomment-1125895287
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#discussion_r873181610
##########
src/lock_manager.cc:
##########
@@ -51,3 +51,7 @@ void LockManager::Lock(const rocksdb::Slice &key) {
void LockManager::UnLock(const rocksdb::Slice &key) {
mutex_pool_[hash(key)]->unlock();
}
+
+void LockManager::LockTwo(const rocksdb::Slice &first_key, const rocksdb::Slice &second_key) {
+ std::lock(*mutex_pool_[hash(first_key)], *mutex_pool_[hash(second_key)]);
Review Comment:
@git-hulk Thanks! It looks like a good solution against a deadlock.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#discussion_r873168955
##########
src/lock_manager.cc:
##########
@@ -51,3 +51,7 @@ void LockManager::Lock(const rocksdb::Slice &key) {
void LockManager::UnLock(const rocksdb::Slice &key) {
mutex_pool_[hash(key)]->unlock();
}
+
+void LockManager::LockTwo(const rocksdb::Slice &first_key, const rocksdb::Slice &second_key) {
+ std::lock(*mutex_pool_[hash(first_key)], *mutex_pool_[hash(second_key)]);
Review Comment:
This way may unlock the wrong mutex? coz the unlock method didn't recalculate the mutex index. After reconsidering, to avoid deadlock that we need to do below things:
1. Do NOT acquire the same lock in the single thread
2. Acquire locks with the hash index order, this may also cause deadlock if one thread acquired the `keys A B` and another acquired `B A`
Maybe I can try to implement multi locks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#discussion_r873168955
##########
src/lock_manager.cc:
##########
@@ -51,3 +51,7 @@ void LockManager::Lock(const rocksdb::Slice &key) {
void LockManager::UnLock(const rocksdb::Slice &key) {
mutex_pool_[hash(key)]->unlock();
}
+
+void LockManager::LockTwo(const rocksdb::Slice &first_key, const rocksdb::Slice &second_key) {
+ std::lock(*mutex_pool_[hash(first_key)], *mutex_pool_[hash(second_key)]);
Review Comment:
This way may unlock the wrong mutex? coz the unlock method didn't recalculate the mutex index. After reconsidering, to avoid deadlock that we need to do below things:
1. NOT acquire the same lock
2. Acquire locks with the hash index order, this may also cause deadlock if one thread acquired the `keys A B` and another acquired `B A`
Maybe I can try to implement multi locks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#discussion_r873170655
##########
src/lock_manager.cc:
##########
@@ -51,3 +51,7 @@ void LockManager::Lock(const rocksdb::Slice &key) {
void LockManager::UnLock(const rocksdb::Slice &key) {
mutex_pool_[hash(key)]->unlock();
}
+
+void LockManager::LockTwo(const rocksdb::Slice &first_key, const rocksdb::Slice &second_key) {
+ std::lock(*mutex_pool_[hash(first_key)], *mutex_pool_[hash(second_key)]);
Review Comment:
👍 It's a bit complex, I'm very happy to have a try and submit the PR to your branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT commented on a diff in pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on code in PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#discussion_r873437597
##########
src/lock_manager.h:
##########
@@ -55,3 +58,25 @@ class LockGuard {
LockManager *lock_mgr_ = nullptr;
rocksdb::Slice key_;
};
+
+class MultiLockGuard {
+ public:
+ explicit MultiLockGuard(LockManager *lock_mgr, const std::vector<std::string> &keys):
+ lock_mgr_(lock_mgr) {
+ locks_ = lock_mgr_->MultiGet(keys);
+ for (const auto &iter : locks_) {
+ iter->lock();
+ }
+ }
+
+ ~MultiLockGuard() {
+ // Lock with order `A B C` and unlock should be `C B A`
Review Comment:
why we need to unlock reversely, AFAIK, they are separate.
##########
Changelog:
##########
@@ -1,5 +1,8 @@
# Version 999.999.999
+New features
+ - Add LMove command
+
Review Comment:
```suggestion
```
you don't need to modify this file, i will do this when we release new version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT commented on a diff in pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on code in PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#discussion_r873466888
##########
Changelog:
##########
@@ -1,5 +1,8 @@
# Version 999.999.999
+New features
+ - Add LMove command
+
Review Comment:
no sorry, i think "commit suggestion" just provide a alternative easy way to apply CRs without changing and pushing on the local machine, but everyone has their own preferences, it is not a problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#discussion_r873170142
##########
src/lock_manager.cc:
##########
@@ -51,3 +51,7 @@ void LockManager::Lock(const rocksdb::Slice &key) {
void LockManager::UnLock(const rocksdb::Slice &key) {
mutex_pool_[hash(key)]->unlock();
}
+
+void LockManager::LockTwo(const rocksdb::Slice &first_key, const rocksdb::Slice &second_key) {
+ std::lock(*mutex_pool_[hash(first_key)], *mutex_pool_[hash(second_key)]);
Review Comment:
> This way may unlock the wrong mutex?
You are right. My bad.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#discussion_r873165180
##########
src/lock_manager.cc:
##########
@@ -51,3 +51,7 @@ void LockManager::Lock(const rocksdb::Slice &key) {
void LockManager::UnLock(const rocksdb::Slice &key) {
mutex_pool_[hash(key)]->unlock();
}
+
+void LockManager::LockTwo(const rocksdb::Slice &first_key, const rocksdb::Slice &second_key) {
+ std::lock(*mutex_pool_[hash(first_key)], *mutex_pool_[hash(second_key)]);
Review Comment:
I added a check that will track the case when two indexes from the mutex pool are the same. In that case, the second index will be increased to point to reference different mutex. Does it have any drawbacks?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#issuecomment-1129846166
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#issuecomment-1126717780
OK, thanks. Will have a look soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#discussion_r872992590
##########
src/lock_manager.cc:
##########
@@ -51,3 +51,7 @@ void LockManager::Lock(const rocksdb::Slice &key) {
void LockManager::UnLock(const rocksdb::Slice &key) {
mutex_pool_[hash(key)]->unlock();
}
+
+void LockManager::LockTwo(const rocksdb::Slice &first_key, const rocksdb::Slice &second_key) {
+ std::lock(*mutex_pool_[hash(first_key)], *mutex_pool_[hash(second_key)]);
Review Comment:
In my implementation, if the source and destination are the same LockManager::LockTwo won't be called. Instead, only one mutex will be locked via LockGuard. Additionally, `TwoLockGuard` or `LockTwo()` can be modified to check whether two keys are equal and make a proper lock.
Yes, I was thinking about multi-lock, but there are 2 points:
- is it possible that more than 2 locks should be locked?
- multi-version should deal with templates :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#discussion_r873157039
##########
src/lock_manager.cc:
##########
@@ -51,3 +51,7 @@ void LockManager::Lock(const rocksdb::Slice &key) {
void LockManager::UnLock(const rocksdb::Slice &key) {
mutex_pool_[hash(key)]->unlock();
}
+
+void LockManager::LockTwo(const rocksdb::Slice &first_key, const rocksdb::Slice &second_key) {
+ std::lock(*mutex_pool_[hash(first_key)], *mutex_pool_[hash(second_key)]);
Review Comment:
Unfortunately, I can't implement `MultiLockGuard` because of a lack of experience working with templates and variable argument functions.
> Aha, we are using crc(key)%buckets to calculate the hash value, so different keys can't promise the value always be different.
In this case, I think, even in the case of implementing `MultiLockGuard` the issue of a possible deadlock will be present.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT commented on pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#issuecomment-1131072511
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT merged pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
ShooterIT merged PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #577: Add LMove command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #577:
URL: https://github.com/apache/incubator-kvrocks/pull/577#issuecomment-1125921849
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org