jihuayu commented on code in PR #2222:
URL: https://github.com/apache/kvrocks/pull/2222#discussion_r1577532298
##########
src/commands/cmd_server.cc:
##########
@@ -878,7 +878,7 @@ class CommandScan : public CommandScanBase {
std::vector<std::string> keys;
std::string end_key;
- auto s = redis_db.Scan(key_name, limit_, prefix_, &keys, &end_key);
+ auto s = redis_db.Scan(key_name, limit_, srv->GetConfig()->max_scan_num,
prefix_, &keys, &end_key, pm_);
Review Comment:
This code looks ugly. Can we have some good way to deal with config?
##########
src/storage/redis_db.cc:
##########
@@ -359,28 +363,57 @@ rocksdb::Status Database::Scan(const std::string &cursor,
uint64_t limit, const
}
uint16_t slot_id = slot_start;
+
+ auto scan_match_check = [&]() -> bool {
+ if (ns_prefix.empty()) {
+ return true;
+ }
+ if (!iter->key().starts_with(ns_prefix)) {
+ return false;
+ }
+ auto key_view = iter->key().ToStringView();
+ auto sub_key_view = static_cast<Slice>(key_view.substr(ns_prefix.size(),
key_view.size() - ns_prefix.size()));
+ if (pm == 1 && !sub_key_view.ends_with(fix)) {
+ return false;
+ } else if (pm == 2 && sub_key_view.ToStringView().find(fix) ==
std::string::npos) {
+ return false;
+ }
+ return true;
+ };
+
while (true) {
- for (; iter->Valid() && cnt < limit; iter->Next()) {
- if (!ns_prefix.empty() && !iter->key().starts_with(ns_prefix)) {
- break;
+ for (; iter->Valid() && cnt_success < scan_success_limit; iter->Next()) {
+ if (!scan_match_check()) {
+ cnt_false++;
+ if (cnt_false < scan_false_limit) {
+ continue;
+ }
}
+ // The user_key must be updated either when the scan is successful, or
when the number of consecutive failures
+ // reaches its limit
Metadata metadata(kRedisNone, false);
auto s = metadata.Decode(iter->value());
if (!s.ok()) continue;
if (metadata.Expired()) continue;
std::tie(std::ignore, user_key) =
ExtractNamespaceKey<std::string>(iter->key(), storage_->IsSlotIdEncoded());
+ if (cnt_false >= scan_false_limit) {
+ break;
+ }
keys->emplace_back(user_key);
- cnt++;
+ cnt_success++;
}
- if (!storage_->IsSlotIdEncoded() || prefix.empty()) {
- if (!keys->empty() && cnt >= limit) {
+ if (!storage_->IsSlotIdEncoded() || fix.empty()) {
+ if (!keys->empty() && (cnt_success >= scan_success_limit)) {
+ end_cursor->append(user_key);
+ }
+ if (keys->empty() && cnt_false >= scan_false_limit) {
end_cursor->append(user_key);
}
Review Comment:
There may be a situation where some keys have been found, but `cnt_false >=
scan_false_limit`.
##########
src/storage/redis_db.cc:
##########
@@ -359,28 +363,57 @@ rocksdb::Status Database::Scan(const std::string &cursor,
uint64_t limit, const
}
uint16_t slot_id = slot_start;
+
+ auto scan_match_check = [&]() -> bool {
+ if (ns_prefix.empty()) {
+ return true;
+ }
+ if (!iter->key().starts_with(ns_prefix)) {
+ return false;
+ }
+ auto key_view = iter->key().ToStringView();
+ auto sub_key_view = static_cast<Slice>(key_view.substr(ns_prefix.size(),
key_view.size() - ns_prefix.size()));
+ if (pm == 1 && !sub_key_view.ends_with(fix)) {
+ return false;
+ } else if (pm == 2 && sub_key_view.ToStringView().find(fix) ==
std::string::npos) {
+ return false;
+ }
+ return true;
+ };
+
while (true) {
- for (; iter->Valid() && cnt < limit; iter->Next()) {
- if (!ns_prefix.empty() && !iter->key().starts_with(ns_prefix)) {
- break;
+ for (; iter->Valid() && cnt_success < scan_success_limit; iter->Next()) {
+ if (!scan_match_check()) {
+ cnt_false++;
+ if (cnt_false < scan_false_limit) {
+ continue;
+ }
}
+ // The user_key must be updated either when the scan is successful, or
when the number of consecutive failures
+ // reaches its limit
Metadata metadata(kRedisNone, false);
auto s = metadata.Decode(iter->value());
if (!s.ok()) continue;
if (metadata.Expired()) continue;
std::tie(std::ignore, user_key) =
ExtractNamespaceKey<std::string>(iter->key(), storage_->IsSlotIdEncoded());
+ if (cnt_false >= scan_false_limit) {
+ break;
+ }
Review Comment:
Why not put those codes to begin?
```C++
if (!scan_match_check()) {
cnt_false++;
if (cnt_false < scan_false_limit) {
continue;
} else {
break;
}
}
```
##########
src/commands/scan_base.h:
##########
@@ -83,6 +89,7 @@ class CommandScanBase : public Commander {
std::string cursor_;
std::string prefix_;
int limit_ = 20;
+ int pm_ = 0;
Review Comment:
Can you use enum instead of it?
Can you change and rename it to `prefix_mode_`?
##########
src/storage/redis_db.cc:
##########
@@ -322,10 +327,12 @@ rocksdb::Status Database::Keys(const std::string &prefix,
std::vector<std::strin
return rocksdb::Status::OK();
}
-rocksdb::Status Database::Scan(const std::string &cursor, uint64_t limit,
const std::string &prefix,
- std::vector<std::string> *keys, std::string
*end_cursor) {
+rocksdb::Status Database::Scan(const std::string &cursor, uint64_t
scan_success_limit, uint64_t scan_false_limit,
+ const std::string &fix,
std::vector<std::string> *keys, std::string *end_cursor,
+ const int pm) {
Review Comment:
Can you change the param `fix` to a more meaningful name? Can you keep the
name the same as the header file?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
