You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/09/20 09:36:40 UTC
[GitHub] [incubator-kvrocks] ellutionist opened a new pull request, #901: fix: non positive ttl in slot migration
ellutionist opened a new pull request, #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901
PR to solve issue #891
--
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 #901: Support EXAT/PEXAT option in the set command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#issuecomment-1258229840
@ellutionist Can you help to resolve the conflict in redis_cmd.cc
--
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] datavisorchengpeng commented on a diff in pull request #901: fix: non positive ttl in slot migration
Posted by GitBox <gi...@apache.org>.
datavisorchengpeng commented on code in PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#discussion_r975207872
##########
src/slot_migrate.cc:
##########
@@ -568,6 +568,12 @@ Status SlotMigrate::MigrateOneKey(rocksdb::Slice key, std::string *restore_cmds)
return Status(Status::cOK, "empty");;
}
+ int64_t now;
+ rocksdb::Env::Default()->GetCurrentTime(&now);
+ if (metadata.expire <= now) {
Review Comment:
Thank you for reminding me to check that `metadata.expire` must be greater than 0.
The reason why I did not use `metadata.Expired()` is that this method returns `false` when `metadata.expire == now`, which will make the source send a ttl of 0 to the destination, thus ruin the whole slot migration. I happened to meet such case before.
--
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] PragmaTwice commented on a diff in pull request #901: fix: non positive ttl in slot migration
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#discussion_r979987294
##########
src/redis_cmd.cc:
##########
@@ -475,17 +475,34 @@ class CommandSet : public Commander {
} else if (opt == "xx" && !nx_) {
xx_ = true;
} else if (opt == "ex" && !ttl_ && !last_arg) {
- try {
- std::string s = args_[++i];
- std::string::size_type sz;
- ttl_ = std::stoi(s, &sz);
- if (sz != s.size()) {
- return Status(Status::RedisParseErr, errValueNotInterger);
- }
- } catch (std::exception &e) {
+ std::string s = args_[++i];
+ StatusOr<int> parse_status = ParseInt(s);
Review Comment:
```suggestion
auto parse_status = ParseInt<int>(s);
```
--
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] ellutionist commented on a diff in pull request #901: fix: non positive ttl in slot migration
Posted by GitBox <gi...@apache.org>.
ellutionist commented on code in PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#discussion_r975266588
##########
src/slot_migrate.cc:
##########
@@ -568,6 +568,12 @@ Status SlotMigrate::MigrateOneKey(rocksdb::Slice key, std::string *restore_cmds)
return Status(Status::cOK, "empty");;
}
+ int64_t now;
+ rocksdb::Env::Default()->GetCurrentTime(&now);
+ if (metadata.expire <= now) {
Review Comment:
What do you think of:
1. Source node still check whether the key has expired and skip it if it has.
2. Source node employs `SET key value EXAT unix-time-seconds` (not supported yet as far as I know, but not hard to implement) instead of `SET key value EX ttl` to send the exact expire ttl during the migration, in order to address the clock syncing or network latency issue.
3. Target node checks the expire unix time when receiving `EXAT` and skip the key when it is behind the current system clock (Does the response "OK" looks reasonable?). The target node cannot respond an error since the source node will abort the migration upon receiving any error reply.
--
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] ellutionist commented on a diff in pull request #901: fix: non positive ttl in slot migration
Posted by GitBox <gi...@apache.org>.
ellutionist commented on code in PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#discussion_r977798772
##########
src/slot_migrate.cc:
##########
@@ -568,6 +568,12 @@ Status SlotMigrate::MigrateOneKey(rocksdb::Slice key, std::string *restore_cmds)
return Status(Status::cOK, "empty");;
}
+ int64_t now;
+ rocksdb::Env::Default()->GetCurrentTime(&now);
+ if (metadata.expire <= now) {
Review Comment:
Just implemented "EXAT|PXAT" in the [new commit](https://github.com/apache/incubator-kvrocks/pull/901/commits/3a541e57ddbb8389095a6a691e1a234957bfe655). The implementation is trying to keep the change to the minimal and keep the bahavior consistent with the original redis (deleting the key if it should be expired).
--
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] PragmaTwice commented on a diff in pull request #901: fix: non positive ttl in slot migration
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#discussion_r979991422
##########
src/redis_cmd.cc:
##########
@@ -475,17 +475,34 @@ class CommandSet : public Commander {
} else if (opt == "xx" && !nx_) {
xx_ = true;
} else if (opt == "ex" && !ttl_ && !last_arg) {
- try {
- std::string s = args_[++i];
- std::string::size_type sz;
- ttl_ = std::stoi(s, &sz);
- if (sz != s.size()) {
- return Status(Status::RedisParseErr, errValueNotInterger);
- }
- } catch (std::exception &e) {
+ std::string s = args_[++i];
+ StatusOr<int> parse_status = ParseInt(s);
+ if (!parse_status.IsOK()) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ ttl_ = parse_status.GetValue();
if (ttl_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
+ } else if (opt == "exat" && !ttl_ && !expire_ && !last_arg) {
+ std::string s = args_[++i];
+ StatusOr<int64_t> parse_status = ParseInt(s);
+ if (!parse_status.IsOK()) {
+ return Status(Status::RedisParseErr, errValueNotInterger);
+ }
+ expire_ = parse_status.GetValue();
+ if (expire_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
+ } else if (opt == "pxat" && !ttl_ && !expire_ && !last_arg) {
+ std::string s = args_[++i];
+ StatusOr<uint64_t> parse_status = ParseInt(s);
Review Comment:
```suggestion
auto parse_status = ParseInt<uint64_t>(s);
```
--
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 #901: Support EXAT/PEXAT option in the set command
Posted by GitBox <gi...@apache.org>.
git-hulk merged PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901
--
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 #901: Support EXAT/PEXAT option in the set command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#issuecomment-1259581846
Many thanks for your contribution, will merge if there are no more comments by tomorrow. cc @PragmaTwice @caipengbo
--
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] ellutionist commented on a diff in pull request #901: fix: non positive ttl in slot migration
Posted by GitBox <gi...@apache.org>.
ellutionist commented on code in PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#discussion_r980092590
##########
src/redis_cmd.cc:
##########
@@ -475,17 +475,34 @@ class CommandSet : public Commander {
} else if (opt == "xx" && !nx_) {
xx_ = true;
} else if (opt == "ex" && !ttl_ && !last_arg) {
- try {
- std::string s = args_[++i];
- std::string::size_type sz;
- ttl_ = std::stoi(s, &sz);
- if (sz != s.size()) {
- return Status(Status::RedisParseErr, errValueNotInterger);
- }
- } catch (std::exception &e) {
+ std::string s = args_[++i];
+ StatusOr<int> parse_status = ParseInt(s);
+ if (!parse_status.IsOK()) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ ttl_ = parse_status.GetValue();
if (ttl_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
+ } else if (opt == "exat" && !ttl_ && !expire_ && !last_arg) {
+ std::string s = args_[++i];
+ StatusOr<int64_t> parse_status = ParseInt(s);
+ if (!parse_status.IsOK()) {
+ return Status(Status::RedisParseErr, errValueNotInterger);
+ }
+ expire_ = parse_status.GetValue();
+ if (expire_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
+ } else if (opt == "pxat" && !ttl_ && !expire_ && !last_arg) {
+ std::string s = args_[++i];
+ StatusOr<uint64_t> parse_status = ParseInt(s);
Review Comment:
Thanks for correction!
--
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] tanruixiang commented on pull request #901: Support EXAT/PEXAT option in the set command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#issuecomment-1259362238
@ellutionist Thank you very much for your contribution. LGTM. Do you have time to add a unit test for cpp? It should be in `t_string_test.cc`.
--
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] PragmaTwice commented on a diff in pull request #901: fix: non positive ttl in slot migration
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#discussion_r979991083
##########
src/redis_cmd.cc:
##########
@@ -475,17 +475,34 @@ class CommandSet : public Commander {
} else if (opt == "xx" && !nx_) {
xx_ = true;
} else if (opt == "ex" && !ttl_ && !last_arg) {
- try {
- std::string s = args_[++i];
- std::string::size_type sz;
- ttl_ = std::stoi(s, &sz);
- if (sz != s.size()) {
- return Status(Status::RedisParseErr, errValueNotInterger);
- }
- } catch (std::exception &e) {
+ std::string s = args_[++i];
+ StatusOr<int> parse_status = ParseInt(s);
+ if (!parse_status.IsOK()) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ ttl_ = parse_status.GetValue();
if (ttl_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
+ } else if (opt == "exat" && !ttl_ && !expire_ && !last_arg) {
+ std::string s = args_[++i];
+ StatusOr<int64_t> parse_status = ParseInt(s);
Review Comment:
```suggestion
auto parse_status = ParseInt<int64_t>(s);
```
--
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] ellutionist commented on pull request #901: Support EXAT/PEXAT option in the set command
Posted by GitBox <gi...@apache.org>.
ellutionist commented on PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#issuecomment-1259410816
> @ellutionist Thank you very much for your contribution. LGTM. Do you have time to add a unit test for cpp? It should be in `t_string_test.cc`.
@tanruixiang Since this PR does not bring any new interface like `SetEX` or `MSet` to the class `Redis::String`, I cannot see any proper entry point for cpp unit test. Any good idea?
--
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 #901: fix: non positive ttl in slot migration
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#discussion_r975237055
##########
src/slot_migrate.cc:
##########
@@ -568,6 +568,12 @@ Status SlotMigrate::MigrateOneKey(rocksdb::Slice key, std::string *restore_cmds)
return Status(Status::cOK, "empty");;
}
+ int64_t now;
+ rocksdb::Env::Default()->GetCurrentTime(&now);
+ if (metadata.expire <= now) {
Review Comment:
I think there are two issues in this case:
1. Source node should never send known expired keys and we need to check whether is expired or not.
2. Target node should also skip expired keys instead of stoping the migration flow, since we can't guarantee the clock between source and target nodes are always the same and the network delay also may cause TTL = 0 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: 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 #901: fix: non positive ttl in slot migration
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#discussion_r975344459
##########
src/slot_migrate.cc:
##########
@@ -568,6 +568,12 @@ Status SlotMigrate::MigrateOneKey(rocksdb::Slice key, std::string *restore_cmds)
return Status(Status::cOK, "empty");;
}
+ int64_t now;
+ rocksdb::Env::Default()->GetCurrentTime(&now);
+ if (metadata.expire <= now) {
Review Comment:
Yes, exactly. I think it's ok to response ok when it's skipped.
--
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] xiaobiaozhao commented on a diff in pull request #901: fix: non positive ttl in slot migration
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#discussion_r975385685
##########
src/slot_migrate.cc:
##########
@@ -568,6 +568,12 @@ Status SlotMigrate::MigrateOneKey(rocksdb::Slice key, std::string *restore_cmds)
return Status(Status::cOK, "empty");;
}
+ int64_t now;
+ rocksdb::Env::Default()->GetCurrentTime(&now);
+ if (metadata.expire <= now) {
Review Comment:
LGTM
--
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 #901: fix: non positive ttl in slot migration
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#discussion_r977802839
##########
src/slot_migrate.cc:
##########
@@ -568,6 +568,12 @@ Status SlotMigrate::MigrateOneKey(rocksdb::Slice key, std::string *restore_cmds)
return Status(Status::cOK, "empty");;
}
+ int64_t now;
+ rocksdb::Env::Default()->GetCurrentTime(&now);
+ if (metadata.expire <= now) {
Review Comment:
cool, thanks.
--
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] ellutionist commented on pull request #901: Support EXAT/PEXAT option in the set command
Posted by GitBox <gi...@apache.org>.
ellutionist commented on PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#issuecomment-1258286993
> @ellutionist Can you help to resolve the conflict in redis_cmd.cc
Conflict resolved.
--
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 #901: fix: non positive ttl in slot migration
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#discussion_r975144444
##########
src/slot_migrate.cc:
##########
@@ -568,6 +568,12 @@ Status SlotMigrate::MigrateOneKey(rocksdb::Slice key, std::string *restore_cmds)
return Status(Status::cOK, "empty");;
}
+ int64_t now;
+ rocksdb::Env::Default()->GetCurrentTime(&now);
+ if (metadata.expire <= now) {
Review Comment:
expire = 0 means never be expired, can use `metadata.Expired()`.
--
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] ellutionist commented on a diff in pull request #901: fix: non positive ttl in slot migration
Posted by GitBox <gi...@apache.org>.
ellutionist commented on code in PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#discussion_r975209212
##########
src/slot_migrate.cc:
##########
@@ -568,6 +568,12 @@ Status SlotMigrate::MigrateOneKey(rocksdb::Slice key, std::string *restore_cmds)
return Status(Status::cOK, "empty");;
}
+ int64_t now;
+ rocksdb::Env::Default()->GetCurrentTime(&now);
+ if (metadata.expire <= now) {
Review Comment:
Thank you for reminding me to check that `metadata.expire` must be greater than 0.
The reason why I did not use `metadata.Expired()` is that this method returns `false` when `metadata.expire == now`, which will make the source send a ttl of 0 to the destination, thus ruin the whole slot migration. I happened to meet such case before.
--
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] xiaobiaozhao commented on a diff in pull request #901: fix: non positive ttl in slot migration
Posted by GitBox <gi...@apache.org>.
xiaobiaozhao commented on code in PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#discussion_r978167002
##########
src/redis_cmd.cc:
##########
@@ -486,6 +486,36 @@ class CommandSet : public Commander {
return Status(Status::RedisParseErr, errValueNotInterger);
}
if (ttl_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
+ } else if (opt == "exat" && !ttl_ && !expire_ && !last_arg) {
+ try {
+ std::string s = args_[++i];
+ std::string::size_type sz;
+ expire_ = std::stol(s, &sz);
+ if (sz != s.size()) {
+ return Status(Status::RedisParseErr, errValueNotInterger);
+ }
+ } catch (std::exception &e) {
+ return Status(Status::RedisParseErr, errValueNotInterger);
+ }
Review Comment:
```suggestion
} else if (opt == "exat" && !ttl_ && !expire_ && !last_arg) {
std::string s = args_[++i];
std::string::size_type sz;
if (sz != s.size()) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
try {
expire_ = std::stol(s, &sz);
} catch (std::exception &e) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
```
--
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] tanruixiang commented on a diff in pull request #901: fix: non positive ttl in slot migration
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#discussion_r978666839
##########
src/redis_cmd.cc:
##########
@@ -486,6 +486,36 @@ class CommandSet : public Commander {
return Status(Status::RedisParseErr, errValueNotInterger);
}
if (ttl_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
+ } else if (opt == "exat" && !ttl_ && !expire_ && !last_arg) {
+ try {
+ std::string s = args_[++i];
+ std::string::size_type sz;
+ expire_ = std::stol(s, &sz);
Review Comment:
Better to use parseInt<>
##########
src/redis_cmd.cc:
##########
@@ -486,6 +486,36 @@ class CommandSet : public Commander {
return Status(Status::RedisParseErr, errValueNotInterger);
}
if (ttl_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
+ } else if (opt == "exat" && !ttl_ && !expire_ && !last_arg) {
+ try {
+ std::string s = args_[++i];
+ std::string::size_type sz;
+ expire_ = std::stol(s, &sz);
+ if (sz != s.size()) {
+ return Status(Status::RedisParseErr, errValueNotInterger);
+ }
+ } catch (std::exception &e) {
+ return Status(Status::RedisParseErr, errValueNotInterger);
+ }
+ if (expire_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
+ } else if (opt == "pxat" && !ttl_ && !expire_ && !last_arg) {
+ uint64_t expire_ms = 0;
+ try {
+ std::string s = args_[++i];
+ std::string::size_type sz;
+ expire_ms = std::stoul(s, &sz);
Review Comment:
Better to use parseInt<>
--
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] tanruixiang commented on a diff in pull request #901: Support EXAT/PEXAT option in the set command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#discussion_r980742338
##########
src/redis_cmd.cc:
##########
@@ -477,17 +477,34 @@ class CommandSet : public Commander {
} else if (opt == "xx" && !nx_) {
xx_ = true;
} else if (opt == "ex" && !ttl_ && !last_arg) {
- try {
- std::string s = args_[++i];
- std::string::size_type sz;
- ttl_ = std::stoi(s, &sz);
- if (sz != s.size()) {
- return Status(Status::RedisParseErr, errValueNotInterger);
- }
- } catch (std::exception &e) {
+ std::string s = args_[++i];
+ auto parse_status = ParseInt<int>(s);
+ if (!parse_status.IsOK()) {
Review Comment:
```suggestion
if (!parse_status) {
```
##########
src/redis_cmd.cc:
##########
@@ -477,17 +477,34 @@ class CommandSet : public Commander {
} else if (opt == "xx" && !nx_) {
xx_ = true;
} else if (opt == "ex" && !ttl_ && !last_arg) {
- try {
- std::string s = args_[++i];
- std::string::size_type sz;
- ttl_ = std::stoi(s, &sz);
- if (sz != s.size()) {
- return Status(Status::RedisParseErr, errValueNotInterger);
- }
- } catch (std::exception &e) {
+ std::string s = args_[++i];
+ auto parse_status = ParseInt<int>(s);
+ if (!parse_status.IsOK()) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ ttl_ = parse_status.GetValue();
if (ttl_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
+ } else if (opt == "exat" && !ttl_ && !expire_ && !last_arg) {
+ std::string s = args_[++i];
+ auto parse_status = ParseInt<int64_t>(s);
+ if (!parse_status.IsOK()) {
Review Comment:
```suggestion
if (!parse_status) {
```
##########
src/redis_cmd.cc:
##########
@@ -477,17 +477,34 @@ class CommandSet : public Commander {
} else if (opt == "xx" && !nx_) {
xx_ = true;
} else if (opt == "ex" && !ttl_ && !last_arg) {
- try {
- std::string s = args_[++i];
- std::string::size_type sz;
- ttl_ = std::stoi(s, &sz);
- if (sz != s.size()) {
- return Status(Status::RedisParseErr, errValueNotInterger);
- }
- } catch (std::exception &e) {
+ std::string s = args_[++i];
+ auto parse_status = ParseInt<int>(s);
+ if (!parse_status.IsOK()) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ ttl_ = parse_status.GetValue();
if (ttl_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
+ } else if (opt == "exat" && !ttl_ && !expire_ && !last_arg) {
+ std::string s = args_[++i];
+ auto parse_status = ParseInt<int64_t>(s);
+ if (!parse_status.IsOK()) {
+ return Status(Status::RedisParseErr, errValueNotInterger);
+ }
+ expire_ = parse_status.GetValue();
+ if (expire_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
+ } else if (opt == "pxat" && !ttl_ && !expire_ && !last_arg) {
+ std::string s = args_[++i];
+ auto parse_status = ParseInt<uint64_t>(s);
+ if (!parse_status.IsOK()) {
+ return Status(Status::RedisParseErr, errValueNotInterger);
+ }
+ uint64_t expire_ms = parse_status.GetValue();
Review Comment:
```suggestion
uint64_t expire_ms = *parse_status;
```
##########
src/redis_cmd.cc:
##########
@@ -477,17 +477,34 @@ class CommandSet : public Commander {
} else if (opt == "xx" && !nx_) {
xx_ = true;
} else if (opt == "ex" && !ttl_ && !last_arg) {
- try {
- std::string s = args_[++i];
- std::string::size_type sz;
- ttl_ = std::stoi(s, &sz);
- if (sz != s.size()) {
- return Status(Status::RedisParseErr, errValueNotInterger);
- }
- } catch (std::exception &e) {
+ std::string s = args_[++i];
+ auto parse_status = ParseInt<int>(s);
+ if (!parse_status.IsOK()) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ ttl_ = parse_status.GetValue();
if (ttl_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
+ } else if (opt == "exat" && !ttl_ && !expire_ && !last_arg) {
+ std::string s = args_[++i];
+ auto parse_status = ParseInt<int64_t>(s);
+ if (!parse_status.IsOK()) {
+ return Status(Status::RedisParseErr, errValueNotInterger);
+ }
+ expire_ = parse_status.GetValue();
+ if (expire_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
+ } else if (opt == "pxat" && !ttl_ && !expire_ && !last_arg) {
+ std::string s = args_[++i];
+ auto parse_status = ParseInt<uint64_t>(s);
Review Comment:
```suggestion
auto parse_status = ParseInt<int64_t>(args_[++i]);
```
##########
src/redis_cmd.cc:
##########
@@ -477,17 +477,34 @@ class CommandSet : public Commander {
} else if (opt == "xx" && !nx_) {
xx_ = true;
} else if (opt == "ex" && !ttl_ && !last_arg) {
- try {
- std::string s = args_[++i];
- std::string::size_type sz;
- ttl_ = std::stoi(s, &sz);
- if (sz != s.size()) {
- return Status(Status::RedisParseErr, errValueNotInterger);
- }
- } catch (std::exception &e) {
+ std::string s = args_[++i];
+ auto parse_status = ParseInt<int>(s);
+ if (!parse_status.IsOK()) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ ttl_ = parse_status.GetValue();
if (ttl_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
+ } else if (opt == "exat" && !ttl_ && !expire_ && !last_arg) {
+ std::string s = args_[++i];
+ auto parse_status = ParseInt<int64_t>(s);
+ if (!parse_status.IsOK()) {
+ return Status(Status::RedisParseErr, errValueNotInterger);
+ }
+ expire_ = parse_status.GetValue();
Review Comment:
```suggestion
expire_ = *parse_status;
```
##########
src/redis_cmd.cc:
##########
@@ -477,17 +477,34 @@ class CommandSet : public Commander {
} else if (opt == "xx" && !nx_) {
xx_ = true;
} else if (opt == "ex" && !ttl_ && !last_arg) {
- try {
- std::string s = args_[++i];
- std::string::size_type sz;
- ttl_ = std::stoi(s, &sz);
- if (sz != s.size()) {
- return Status(Status::RedisParseErr, errValueNotInterger);
- }
- } catch (std::exception &e) {
+ std::string s = args_[++i];
+ auto parse_status = ParseInt<int>(s);
+ if (!parse_status.IsOK()) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ ttl_ = parse_status.GetValue();
Review Comment:
```suggestion
ttl_ = *parse_status;
```
##########
src/redis_cmd.cc:
##########
@@ -477,17 +477,34 @@ class CommandSet : public Commander {
} else if (opt == "xx" && !nx_) {
xx_ = true;
} else if (opt == "ex" && !ttl_ && !last_arg) {
- try {
- std::string s = args_[++i];
- std::string::size_type sz;
- ttl_ = std::stoi(s, &sz);
- if (sz != s.size()) {
- return Status(Status::RedisParseErr, errValueNotInterger);
- }
- } catch (std::exception &e) {
+ std::string s = args_[++i];
+ auto parse_status = ParseInt<int>(s);
+ if (!parse_status.IsOK()) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ ttl_ = parse_status.GetValue();
if (ttl_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
+ } else if (opt == "exat" && !ttl_ && !expire_ && !last_arg) {
+ std::string s = args_[++i];
+ auto parse_status = ParseInt<int64_t>(s);
Review Comment:
```suggestion
auto parse_status = ParseInt<int64_t>(args_[++i]);
```
##########
src/redis_cmd.cc:
##########
@@ -477,17 +477,34 @@ class CommandSet : public Commander {
} else if (opt == "xx" && !nx_) {
xx_ = true;
} else if (opt == "ex" && !ttl_ && !last_arg) {
- try {
- std::string s = args_[++i];
- std::string::size_type sz;
- ttl_ = std::stoi(s, &sz);
- if (sz != s.size()) {
- return Status(Status::RedisParseErr, errValueNotInterger);
- }
- } catch (std::exception &e) {
+ std::string s = args_[++i];
+ auto parse_status = ParseInt<int>(s);
+ if (!parse_status.IsOK()) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ ttl_ = parse_status.GetValue();
if (ttl_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
+ } else if (opt == "exat" && !ttl_ && !expire_ && !last_arg) {
+ std::string s = args_[++i];
+ auto parse_status = ParseInt<int64_t>(s);
+ if (!parse_status.IsOK()) {
+ return Status(Status::RedisParseErr, errValueNotInterger);
+ }
+ expire_ = parse_status.GetValue();
+ if (expire_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
+ } else if (opt == "pxat" && !ttl_ && !expire_ && !last_arg) {
+ std::string s = args_[++i];
+ auto parse_status = ParseInt<uint64_t>(s);
+ if (!parse_status.IsOK()) {
Review Comment:
```suggestion
if (!parse_status) {
```
--
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] tanruixiang commented on pull request #901: Support EXAT/PEXAT option in the set command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #901:
URL: https://github.com/apache/incubator-kvrocks/pull/901#issuecomment-1259431652
> @tanruixiang Since this PR does not bring any new interface like `SetEX` or `MSet` to the class `Redis::String`, I cannot see any proper entry point for cpp unit test. Any good idea?
Sorry, I revisited the code. You're right. I guess tcl's tests are enough. Thank you very much for your contribution.
--
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