You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "zevin02 (via GitHub)" <gi...@apache.org> on 2023/05/21 12:43:25 UTC
[GitHub] [incubator-kvrocks] zevin02 opened a new pull request, #1453: Feat: optimize MGet function for improved performance
zevin02 opened a new pull request, #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453
fixes: #1441
- Use RocksDB's MultiGet to improve mget performance of hash
--
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: issues-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 #1453: Feat: optimize MGet function for improved performance
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453#issuecomment-1556559248
Thanks all, the patch looks good to be merged.
--
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: issues-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 #1453: Feat: optimize MGet function for improved performance
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453#issuecomment-1556195165
> Sorry, miss the NotFound part
😊 No problem. Great thanks for your review.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] zevin02 commented on pull request #1453: Feat: optimize MGet function for improved performance
Posted by "zevin02 (via GitHub)" <gi...@apache.org>.
zevin02 commented on PR #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453#issuecomment-1556175596
I think I can add it
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk merged pull request #1453: Feat: optimize MGet function for improved performance
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] mapleFU commented on pull request #1453: Feat: optimize MGet function for improved performance
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453#issuecomment-1556219965
@zevin02 Patch LGTM. And the hash case is tested. Let's wait committer rerun the unittest tomorrow.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1453: Feat: optimize MGet function for improved performance
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453#discussion_r1199772992
##########
src/types/redis_hash.cc:
##########
@@ -174,16 +174,27 @@ rocksdb::Status Hash::MGet(const Slice &user_key, const std::vector<Slice> &fiel
rocksdb::ReadOptions read_options;
read_options.snapshot = ss.GetSnapShot();
storage_->SetReadOptions(read_options);
+ std::vector<rocksdb::Slice> keys;
- std::string sub_key, value;
+ keys.reserve(fields.size());
+ std::vector<std::string> sub_keys;
+ sub_keys.resize(fields.size());
+ int i = 0;
for (const auto &field : fields) {
- InternalKey(ns_key, field, metadata.version, storage_->IsSlotIdEncoded()).Encode(&sub_key);
- value.clear();
- s = storage_->Get(read_options, sub_key, &value);
- if (!s.ok() && !s.IsNotFound()) return s;
+ InternalKey(ns_key, field, metadata.version, storage_->IsSlotIdEncoded()).Encode(&(sub_keys[i]));
+ keys.emplace_back(sub_keys[i++]);
+ }
- values->emplace_back(value);
- statuses->emplace_back(s);
+ std::vector<rocksdb::PinnableSlice> values_vector;
+ values_vector.resize(keys.size());
+ std::vector<rocksdb::Status> statuses_vector;
+ statuses_vector.resize(keys.size());
+ storage_->MultiGet(read_options, storage_->GetDB()->DefaultColumnFamily(), keys.size(), keys.data(),
+ values_vector.data(), statuses_vector.data());
+ for (size_t i = 0; i < keys.size(); i++) {
+ if (!statuses_vector[i].ok() && statuses_vector[i].IsNotFound()) return statuses_vector[i];
Review Comment:
This statement corresponds to #1454 1455
##########
src/types/redis_hash.cc:
##########
@@ -174,16 +174,27 @@ rocksdb::Status Hash::MGet(const Slice &user_key, const std::vector<Slice> &fiel
rocksdb::ReadOptions read_options;
read_options.snapshot = ss.GetSnapShot();
storage_->SetReadOptions(read_options);
+ std::vector<rocksdb::Slice> keys;
- std::string sub_key, value;
+ keys.reserve(fields.size());
+ std::vector<std::string> sub_keys;
+ sub_keys.resize(fields.size());
+ int i = 0;
for (const auto &field : fields) {
- InternalKey(ns_key, field, metadata.version, storage_->IsSlotIdEncoded()).Encode(&sub_key);
- value.clear();
- s = storage_->Get(read_options, sub_key, &value);
- if (!s.ok() && !s.IsNotFound()) return s;
+ InternalKey(ns_key, field, metadata.version, storage_->IsSlotIdEncoded()).Encode(&(sub_keys[i]));
+ keys.emplace_back(sub_keys[i++]);
+ }
- values->emplace_back(value);
- statuses->emplace_back(s);
+ std::vector<rocksdb::PinnableSlice> values_vector;
+ values_vector.resize(keys.size());
+ std::vector<rocksdb::Status> statuses_vector;
+ statuses_vector.resize(keys.size());
+ storage_->MultiGet(read_options, storage_->GetDB()->DefaultColumnFamily(), keys.size(), keys.data(),
+ values_vector.data(), statuses_vector.data());
+ for (size_t i = 0; i < keys.size(); i++) {
+ if (!statuses_vector[i].ok() && statuses_vector[i].IsNotFound()) return statuses_vector[i];
Review Comment:
This statement corresponds to #1454
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] infdahai commented on a diff in pull request #1453: Feat: optimize MGet function for improved performance
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453#discussion_r1199772992
##########
src/types/redis_hash.cc:
##########
@@ -174,16 +174,27 @@ rocksdb::Status Hash::MGet(const Slice &user_key, const std::vector<Slice> &fiel
rocksdb::ReadOptions read_options;
read_options.snapshot = ss.GetSnapShot();
storage_->SetReadOptions(read_options);
+ std::vector<rocksdb::Slice> keys;
- std::string sub_key, value;
+ keys.reserve(fields.size());
+ std::vector<std::string> sub_keys;
+ sub_keys.resize(fields.size());
+ int i = 0;
for (const auto &field : fields) {
- InternalKey(ns_key, field, metadata.version, storage_->IsSlotIdEncoded()).Encode(&sub_key);
- value.clear();
- s = storage_->Get(read_options, sub_key, &value);
- if (!s.ok() && !s.IsNotFound()) return s;
+ InternalKey(ns_key, field, metadata.version, storage_->IsSlotIdEncoded()).Encode(&(sub_keys[i]));
+ keys.emplace_back(sub_keys[i++]);
+ }
- values->emplace_back(value);
- statuses->emplace_back(s);
+ std::vector<rocksdb::PinnableSlice> values_vector;
+ values_vector.resize(keys.size());
+ std::vector<rocksdb::Status> statuses_vector;
+ statuses_vector.resize(keys.size());
+ storage_->MultiGet(read_options, storage_->GetDB()->DefaultColumnFamily(), keys.size(), keys.data(),
+ values_vector.data(), statuses_vector.data());
+ for (size_t i = 0; i < keys.size(); i++) {
+ if (!statuses_vector[i].ok() && statuses_vector[i].IsNotFound()) return statuses_vector[i];
Review Comment:
This statement corresponds to #1455
--
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: issues-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 #1453: Feat: optimize MGet function for improved performance
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453#discussion_r1199773528
##########
src/types/redis_hash.cc:
##########
@@ -174,16 +174,27 @@ rocksdb::Status Hash::MGet(const Slice &user_key, const std::vector<Slice> &fiel
rocksdb::ReadOptions read_options;
read_options.snapshot = ss.GetSnapShot();
storage_->SetReadOptions(read_options);
+ std::vector<rocksdb::Slice> keys;
- std::string sub_key, value;
+ keys.reserve(fields.size());
+ std::vector<std::string> sub_keys;
+ sub_keys.resize(fields.size());
+ int i = 0;
for (const auto &field : fields) {
- InternalKey(ns_key, field, metadata.version, storage_->IsSlotIdEncoded()).Encode(&sub_key);
- value.clear();
- s = storage_->Get(read_options, sub_key, &value);
- if (!s.ok() && !s.IsNotFound()) return s;
+ InternalKey(ns_key, field, metadata.version, storage_->IsSlotIdEncoded()).Encode(&(sub_keys[i]));
+ keys.emplace_back(sub_keys[i++]);
+ }
- values->emplace_back(value);
- statuses->emplace_back(s);
+ std::vector<rocksdb::PinnableSlice> values_vector;
+ values_vector.resize(keys.size());
+ std::vector<rocksdb::Status> statuses_vector;
+ statuses_vector.resize(keys.size());
+ storage_->MultiGet(read_options, storage_->GetDB()->DefaultColumnFamily(), keys.size(), keys.data(),
+ values_vector.data(), statuses_vector.data());
+ for (size_t i = 0; i < keys.size(); i++) {
+ if (!statuses_vector[i].ok() && statuses_vector[i].IsNotFound()) return statuses_vector[i];
Review Comment:
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: issues-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 #1453: Feat: optimize MGet function for improved performance
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453#discussion_r1199772620
##########
src/types/redis_hash.cc:
##########
@@ -174,16 +174,27 @@ rocksdb::Status Hash::MGet(const Slice &user_key, const std::vector<Slice> &fiel
rocksdb::ReadOptions read_options;
read_options.snapshot = ss.GetSnapShot();
storage_->SetReadOptions(read_options);
+ std::vector<rocksdb::Slice> keys;
- std::string sub_key, value;
+ keys.reserve(fields.size());
+ std::vector<std::string> sub_keys;
+ sub_keys.resize(fields.size());
+ int i = 0;
for (const auto &field : fields) {
- InternalKey(ns_key, field, metadata.version, storage_->IsSlotIdEncoded()).Encode(&sub_key);
- value.clear();
- s = storage_->Get(read_options, sub_key, &value);
- if (!s.ok() && !s.IsNotFound()) return s;
+ InternalKey(ns_key, field, metadata.version, storage_->IsSlotIdEncoded()).Encode(&(sub_keys[i]));
+ keys.emplace_back(sub_keys[i++]);
+ }
- values->emplace_back(value);
- statuses->emplace_back(s);
+ std::vector<rocksdb::PinnableSlice> values_vector;
+ values_vector.resize(keys.size());
+ std::vector<rocksdb::Status> statuses_vector;
+ statuses_vector.resize(keys.size());
+ storage_->MultiGet(read_options, storage_->GetDB()->DefaultColumnFamily(), keys.size(), keys.data(),
+ values_vector.data(), statuses_vector.data());
+ for (size_t i = 0; i < keys.size(); i++) {
+ if (!statuses_vector[i].ok() && statuses_vector[i].IsNotFound()) return statuses_vector[i];
Review Comment:
```suggestion
if (!statuses_vector[i].ok() && !statuses_vector[i].IsNotFound()) return statuses_vector[i];
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] mapleFU commented on pull request #1453: Feat: optimize MGet function for improved performance
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453#issuecomment-1556192136
@zevin02 Could you find or add a testcase for https://github.com/apache/incubator-kvrocks/issues/1455 ?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] zevin02 commented on pull request #1453: Feat: optimize MGet function for improved performance
Posted by "zevin02 (via GitHub)" <gi...@apache.org>.
zevin02 commented on PR #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453#issuecomment-1556225023
> > What does it mean? Do I need to add a test case for reproducing this issue? I think the PR I just modified already fixed this issue.
>
> It means you can just write a test to record this process. (Add a test to the hash test)
>
> https://github.com/apache/incubator-kvrocks/blob/fb0e3d42031a6e0691c7a736711d0a01d72bdb1a/tests/gocase/unit/type/hash/hash_test.go#L53
>
> The test is just used to avoid this situation reproducing due to the code that follows.
I can add this test case 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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] zevin02 commented on pull request #1453: Feat: optimize MGet function for improved performance
Posted by "zevin02 (via GitHub)" <gi...@apache.org>.
zevin02 commented on PR #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453#issuecomment-1556193028
> @zevin02 Could you find or add a testcase for #1455 ? @zevin02 你能为 #1455 找到或添加一个测试用例吗？
What does it mean? Do I need to add a test case for reproducing this issue? I think the PR I just modified already fixed this issue.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] mapleFU commented on pull request #1453: Feat: optimize MGet function for improved performance
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453#issuecomment-1556211033
```
--- FAIL: TestSlotMigrateSync (47.71s)
--- FAIL: TestSlotMigrateSync/MIGRATE_-_Migrate_sync_with_(or_without)_all_kinds_of_timeouts (0.01s)
slotmigrate_test.go:425:
Error Trace: /home/runner/work/incubator-kvrocks/incubator-kvrocks/tests/gocase/integration/slotmigrate/slotmigrate_test.go:425
Error: Not equal:
expected: string("OK")
actual : <nil>(<nil>)
Test: TestSlotMigrateSync/MIGRATE_-_Migrate_sync_with_(or_without)_all_kinds_of_timeouts
FAIL
exit status 1
```
Is the unittest failed related?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] infdahai commented on pull request #1453: Feat: optimize MGet function for improved performance
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on PR #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453#issuecomment-1556196120
> What does it mean? Do I need to add a test case for reproducing this issue? I think the PR I just modified already fixed this issue.
It means you can just write a test to record this process. (Add a test to the hash test)
https://github.com/apache/incubator-kvrocks/blob/fb0e3d42031a6e0691c7a736711d0a01d72bdb1a/tests/gocase/unit/type/hash/hash_test.go#L53
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] zevin02 commented on pull request #1453: Feat: optimize MGet function for improved performance
Posted by "zevin02 (via GitHub)" <gi...@apache.org>.
zevin02 commented on PR #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453#issuecomment-1556213117
> ```
> --- FAIL: TestSlotMigrateSync (47.71s)
> --- FAIL: TestSlotMigrateSync/MIGRATE_-_Migrate_sync_with_(or_without)_all_kinds_of_timeouts (0.01s)
> slotmigrate_test.go:425:
> Error Trace: /home/runner/work/incubator-kvrocks/incubator-kvrocks/tests/gocase/integration/slotmigrate/slotmigrate_test.go:425
> Error: Not equal:
> expected: string("OK")
> actual : <nil>(<nil>)
> Test: TestSlotMigrateSync/MIGRATE_-_Migrate_sync_with_(or_without)_all_kinds_of_timeouts
> FAIL
> exit status 1
> ```
>
> Is the unittest failed related?
I did not make any changes to the relevant code and I don't know why there is an error.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] zevin02 commented on pull request #1453: Feat: optimize MGet function for improved performance
Posted by "zevin02 (via GitHub)" <gi...@apache.org>.
zevin02 commented on PR #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453#issuecomment-1556215547
emm,So what should I do
> It is related to #1452.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] infdahai commented on pull request #1453: Feat: optimize MGet function for improved performance
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on PR #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453#issuecomment-1556213347
It is related to #1452.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] infdahai commented on pull request #1453: Feat: optimize MGet function for improved performance
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on PR #1453:
URL: https://github.com/apache/incubator-kvrocks/pull/1453#issuecomment-1556217304
Sometimes the test works and sometimes it doesn't. So just ignore it for now.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org