From f02029abbe440ac508c5b7361259bf5cfd1d613b Mon Sep 17 00:00:00 2001 From: ellutionist Date: Tue, 20 Sep 2022 16:06:07 +0800 Subject: [PATCH 1/7] fix: non positive ttl in slot migration --- src/slot_migrate.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/slot_migrate.cc b/src/slot_migrate.cc index a65a3cbb479..f8a31716dcf 100644 --- a/src/slot_migrate.cc +++ b/src/slot_migrate.cc @@ -554,6 +554,12 @@ Status SlotMigrate::MigrateOneKey(const rocksdb::Slice &key, const rocksdb::Slic return Status(Status::cOK, "empty"); } + int64_t now; + rocksdb::Env::Default()->GetCurrentTime(&now); + if (metadata.expire <= now) { + return Status(Status::cOK, "expired"); + } + // Construct command according to type of the key switch (metadata.Type()) { case kRedisString: { From 5dfee330e668d46580d9a8f7b6152f026a131807 Mon Sep 17 00:00:00 2001 From: ellutionist Date: Tue, 20 Sep 2022 18:59:02 +0800 Subject: [PATCH 2/7] check meta.expire greater than zero --- src/slot_migrate.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/slot_migrate.cc b/src/slot_migrate.cc index f8a31716dcf..ce6f816c603 100644 --- a/src/slot_migrate.cc +++ b/src/slot_migrate.cc @@ -554,10 +554,12 @@ Status SlotMigrate::MigrateOneKey(const rocksdb::Slice &key, const rocksdb::Slic return Status(Status::cOK, "empty"); } - int64_t now; - rocksdb::Env::Default()->GetCurrentTime(&now); - if (metadata.expire <= now) { - return Status(Status::cOK, "expired"); + if (metadata.expire > 0) { + int64_t now; + rocksdb::Env::Default()->GetCurrentTime(&now); + if (metadata.expire <= now) { + return Status(Status::cOK, "expired"); + } } // Construct command according to type of the key From 469fd84426b1210d867c153cc78296ba9821d4aa Mon Sep 17 00:00:00 2001 From: ellutionist Date: Thu, 22 Sep 2022 22:54:29 +0800 Subject: [PATCH 3/7] implement "set exat|pxat" && employ in migration --- src/redis_cmd.cc | 43 +++++++++++++++++++++++++++++++++++++++++++ src/slot_migrate.cc | 15 ++++----------- 2 files changed, 47 insertions(+), 11 deletions(-) diff --git a/src/redis_cmd.cc b/src/redis_cmd.cc index 9e6d1771f8d..a0954a4f9ae 100644 --- a/src/redis_cmd.cc +++ b/src/redis_cmd.cc @@ -488,6 +488,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); + if (sz != s.size()) { + return Status(Status::RedisParseErr, errValueNotInterger); + } + } catch (std::exception &e) { + return Status(Status::RedisParseErr, errValueNotInterger); + } + if (expire_ms <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime); + if (expire_ms < 1000) { + expire_ = 1; + } else { + expire_ = static_cast(expire_ms/1000); + } } else if (opt == "px" && !ttl_ && !last_arg) { int64_t ttl_ms = 0; std::string s = args_[++i]; @@ -512,6 +542,18 @@ class CommandSet : public Commander { int ret; Redis::String string_db(svr->storage_, conn->GetNamespace()); rocksdb::Status s; + + if (!ttl_ && expire_) { + int64_t now; + rocksdb::Env::Default()->GetCurrentTime(&now); + ttl_ = expire_ - now; + if (ttl_ <= 0) { + string_db.Del(args_[1]); + *output = Redis::SimpleString("OK"); + return Status::OK(); + } + } + if (nx_) { s = string_db.SetNX(args_[1], args_[2], ttl_, &ret); } else if (xx_) { @@ -534,6 +576,7 @@ class CommandSet : public Commander { bool xx_ = false; bool nx_ = false; int ttl_ = 0; + int64_t expire_ = 0; }; class CommandSetEX : public Commander { diff --git a/src/slot_migrate.cc b/src/slot_migrate.cc index ce6f816c603..c52571079e4 100644 --- a/src/slot_migrate.cc +++ b/src/slot_migrate.cc @@ -554,12 +554,8 @@ Status SlotMigrate::MigrateOneKey(const rocksdb::Slice &key, const rocksdb::Slic return Status(Status::cOK, "empty"); } - if (metadata.expire > 0) { - int64_t now; - rocksdb::Env::Default()->GetCurrentTime(&now); - if (metadata.expire <= now) { - return Status(Status::cOK, "expired"); - } + if (metadata.Expired()) { + return Status(Status::cOK, "expired"); } // Construct command according to type of the key @@ -595,11 +591,8 @@ bool SlotMigrate::MigrateSimpleKey(const rocksdb::Slice &key, const Metadata &me const std::string &bytes, std::string *restore_cmds) { std::vector command = {"set", key.ToString(), bytes.substr(5, bytes.size() - 5)}; if (metadata.expire > 0) { - int64_t now; - rocksdb::Env::Default()->GetCurrentTime(&now); - int32_t ttl = metadata.expire - uint32_t(now); - command.emplace_back("EX"); - command.emplace_back(std::to_string(ttl)); + command.emplace_back("EXAT"); + command.emplace_back(std::to_string(metadata.expire)); } *restore_cmds += Redis::MultiBulkString(command, false); current_pipeline_size_++; From 33121ef38865e879dd0e9daa4d4901b932c6a72e Mon Sep 17 00:00:00 2001 From: ellutionist Date: Fri, 23 Sep 2022 11:37:34 +0800 Subject: [PATCH 4/7] test: cases for EXAT|PXAT && slot migrate --- tests/tcl/tests/integration/slotmigrate.tcl | 8 +++ tests/tcl/tests/unit/type/string.tcl | 63 +++++++++++++++++---- 2 files changed, 59 insertions(+), 12 deletions(-) diff --git a/tests/tcl/tests/integration/slotmigrate.tcl b/tests/tcl/tests/integration/slotmigrate.tcl index aa949447abf..ff662779588 100644 --- a/tests/tcl/tests/integration/slotmigrate.tcl +++ b/tests/tcl/tests/integration/slotmigrate.tcl @@ -147,6 +147,7 @@ start_server {tags {"Src migration server"} overrides {cluster-enabled yes}} { # Set keys set slot1_tag [lindex $::CRC16_SLOT_TABLE 1] set slot1_key_string string_{$slot1_tag} + set slot1_key_string2 string2_{$slot1_tag} set slot1_key_list list_{$slot1_tag} set slot1_key_hash hash_{$slot1_tag} set slot1_key_set set_{$slot1_tag} @@ -168,6 +169,11 @@ start_server {tags {"Src migration server"} overrides {cluster-enabled yes}} { $r0 set $slot1_key_string $slot1_key_string $r0 expire $slot1_key_string 10000 + # Expired string key + $r0 set $slot1_key_string2 $slot1_key_string2 ex 1 + after 3000 + assert_equal [$r0 get $slot1_key_string2] {} + # Type: list $r0 rpush $slot1_key_list 0 1 2 3 4 5 $r0 lpush $slot1_key_list 9 3 7 3 5 4 @@ -242,6 +248,8 @@ start_server {tags {"Src migration server"} overrides {cluster-enabled yes}} { set expire_time [$r1 ttl $slot1_key_string] assert {$expire_time > 1000 && $expire_time <= 10000} + assert_equal [$r1 get $slot1_key_string2] {} + # Check list and expired time assert {[$r1 lrange $slot1_key_list 0 -1] eq $lvalue} set expire_time [$r1 ttl $slot1_key_list] diff --git a/tests/tcl/tests/unit/type/string.tcl b/tests/tcl/tests/unit/type/string.tcl index 7db12afd316..4f51e0b34ae 100644 --- a/tests/tcl/tests/unit/type/string.tcl +++ b/tests/tcl/tests/unit/type/string.tcl @@ -556,17 +556,41 @@ start_server {tags {"string"}} { assert {$ttl <= 10 && $ttl > 5} } - # test "Extended SET EXAT option" { - # r del foo - # r set foo bar exat [expr [clock seconds] + 10] - # assert_range [r ttl foo] 5 10 - # } + test {Extended SET EXAT option} { + r del foo + r set foo bar exat [expr [clock seconds] + 10] + assert_range [r ttl foo] 5 10 + } - # test "Extended SET PXAT option" { - # r del foo - # r set foo bar pxat [expr [clock milliseconds] + 10000] - # assert_range [r ttl foo] 5 10 - # } + test {Extended SET EXAT option with expired timestamp} { + r del foo + assert_equal [r set foo bar exat 1] OK + assert_equal [r get foo] {} + + r set foo bar + assert_equal [r get foo] bar + + assert_equal [r set foo bar exat [expr [clock seconds] - 5]] OK + assert_equal [r get foo] {} + } + + test {Extended SET PXAT option} { + r del foo + r set foo bar pxat [expr [clock milliseconds] + 10000] + assert_range [r ttl foo] 5 10 + } + + test {Extended SET PXAT option with expired timestamp} { + r del foo + assert_equal [r set foo bar pxat 1] OK + assert_equal [r get foo] {} + + r set foo bar + assert_equal [r get foo] bar + + assert_equal [r set foo bar pxat [expr [clock milliseconds] - 5000]] OK + assert_equal [r get foo] {} + } test {Extended SET with incorrect use of multi options should result in syntax err} { catch {r set foo bar ex 10 px 10000} err1 @@ -576,8 +600,23 @@ start_server {tags {"string"}} { test {Extended SET with incorrect expire value} { catch {r set foo bar ex 1234xyz} e - set e - } {*not an integer*} + assert_match {*not an integer*} $e + + catch {r set foo bar ex 0} e + assert_match {*invalid expire time*} $e + + catch {r set foo bar exat 1234xyz} e + assert_match {*not an integer*} $e + + catch {r set foo bar exat 0} e + assert_match {*invalid expire time*} $e + + catch {r set foo bar pxat 1234xyz} e + assert_match {*not an integer*} $e + + catch {r set foo bar pxat 0} e + assert_match {*invalid expire time*} $e + } test {Extended SET using multiple options at once} { r set foo val From 8398e7916a7635a43928fbdd7769836db2999c8c Mon Sep 17 00:00:00 2001 From: ellutionist Date: Mon, 26 Sep 2022 20:34:56 +0800 Subject: [PATCH 5/7] use ParseInt --- src/redis_cmd.cc | 37 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 25 deletions(-) diff --git a/src/redis_cmd.cc b/src/redis_cmd.cc index a0954a4f9ae..546ebb26d30 100644 --- a/src/redis_cmd.cc +++ b/src/redis_cmd.cc @@ -477,41 +477,28 @@ 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 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) { - 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) { + std::string s = args_[++i]; + StatusOr 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) { - uint64_t expire_ms = 0; - try { - std::string s = args_[++i]; - std::string::size_type sz; - expire_ms = std::stoul(s, &sz); - if (sz != s.size()) { - return Status(Status::RedisParseErr, errValueNotInterger); - } - } catch (std::exception &e) { + std::string s = args_[++i]; + StatusOr parse_status = ParseInt(s); + if (!parse_status.IsOK()) { return Status(Status::RedisParseErr, errValueNotInterger); } + uint64_t expire_ms = parse_status.GetValue(); if (expire_ms <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime); if (expire_ms < 1000) { expire_ = 1; From cb6d1d8960eced12ec22ffcb40f57f8a81a8a03c Mon Sep 17 00:00:00 2001 From: ellutionist Date: Mon, 26 Sep 2022 22:15:47 +0800 Subject: [PATCH 6/7] fix template misuse --- src/redis_cmd.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/redis_cmd.cc b/src/redis_cmd.cc index 546ebb26d30..f95dee0d95a 100644 --- a/src/redis_cmd.cc +++ b/src/redis_cmd.cc @@ -478,7 +478,7 @@ class CommandSet : public Commander { xx_ = true; } else if (opt == "ex" && !ttl_ && !last_arg) { std::string s = args_[++i]; - StatusOr parse_status = ParseInt(s); + auto parse_status = ParseInt(s); if (!parse_status.IsOK()) { return Status(Status::RedisParseErr, errValueNotInterger); } @@ -486,7 +486,7 @@ class CommandSet : public Commander { if (ttl_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime); } else if (opt == "exat" && !ttl_ && !expire_ && !last_arg) { std::string s = args_[++i]; - StatusOr parse_status = ParseInt(s); + auto parse_status = ParseInt(s); if (!parse_status.IsOK()) { return Status(Status::RedisParseErr, errValueNotInterger); } @@ -494,7 +494,7 @@ class CommandSet : public Commander { if (expire_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime); } else if (opt == "pxat" && !ttl_ && !expire_ && !last_arg) { std::string s = args_[++i]; - StatusOr parse_status = ParseInt(s); + auto parse_status = ParseInt(s); if (!parse_status.IsOK()) { return Status(Status::RedisParseErr, errValueNotInterger); } From 73a27c3a547c807c180fb9e3cd8cd2dcc80eedb2 Mon Sep 17 00:00:00 2001 From: ellutionist Date: Tue, 27 Sep 2022 13:50:17 +0800 Subject: [PATCH 7/7] make style more concise --- src/redis_cmd.cc | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/redis_cmd.cc b/src/redis_cmd.cc index f95dee0d95a..febe5e9b823 100644 --- a/src/redis_cmd.cc +++ b/src/redis_cmd.cc @@ -477,28 +477,25 @@ class CommandSet : public Commander { } else if (opt == "xx" && !nx_) { xx_ = true; } else if (opt == "ex" && !ttl_ && !last_arg) { - std::string s = args_[++i]; - auto parse_status = ParseInt(s); - if (!parse_status.IsOK()) { + auto parse_result = ParseInt(args_[++i], 10); + if (!parse_result) { return Status(Status::RedisParseErr, errValueNotInterger); } - ttl_ = parse_status.GetValue(); + ttl_ = *parse_result; 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(s); - if (!parse_status.IsOK()) { + auto parse_result = ParseInt(args_[++i], 10); + if (!parse_result) { return Status(Status::RedisParseErr, errValueNotInterger); } - expire_ = parse_status.GetValue(); + expire_ = *parse_result; 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(s); - if (!parse_status.IsOK()) { + auto parse_result = ParseInt(args[++i], 10); + if (!parse_result) { return Status(Status::RedisParseErr, errValueNotInterger); } - uint64_t expire_ms = parse_status.GetValue(); + uint64_t expire_ms = *parse_result; if (expire_ms <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime); if (expire_ms < 1000) { expire_ = 1;