From 6e5de7ac596dc560d474fc10c1e0ee9793eefe45 Mon Sep 17 00:00:00 2001 From: Roman Gershman Date: Wed, 13 Apr 2022 11:52:59 +0300 Subject: [PATCH] Passover cleanups. 1. Add ttl with reload test. 2. Removed several LOG(FATAL) messages and replaced them with error propagation. 3. Added scan test for all the options. --- src/facade/resp_expr.h | 2 +- src/server/error.h | 2 +- src/server/generic_family_test.cc | 29 +++++++++++++++++++++++++++++ src/server/list_family.cc | 21 +++++++++++---------- src/server/rdb_load.cc | 12 ++++++++---- src/server/rdb_save.cc | 4 ++-- src/server/rdb_test.cc | 9 ++++++++- src/server/test_utils.cc | 11 ++++++++--- src/server/test_utils.h | 2 +- src/server/zset_family.cc | 20 ++++++++------------ 10 files changed, 77 insertions(+), 35 deletions(-) diff --git a/src/facade/resp_expr.h b/src/facade/resp_expr.h index fe535dbe6..f927db64a 100644 --- a/src/facade/resp_expr.h +++ b/src/facade/resp_expr.h @@ -20,7 +20,7 @@ class RespExpr { using Vec = std::vector; Type type; - bool has_support; // whether pointers in this item are supported by external storage. + bool has_support; // whether pointers in this item are supported by the external storage. std::variant u; diff --git a/src/server/error.h b/src/server/error.h index 6125ed5eb..e08344242 100644 --- a/src/server/error.h +++ b/src/server/error.h @@ -33,7 +33,7 @@ namespace rdb { enum errc { wrong_signature = 1, bad_version = 2, - module_not_supported = 3, + feature_not_supported = 3, duplicate_key = 4, rdb_file_corrupted = 5, bad_checksum = 6, diff --git a/src/server/generic_family_test.cc b/src/server/generic_family_test.cc index ca2f10493..3b4185173 100644 --- a/src/server/generic_family_test.cc +++ b/src/server/generic_family_test.cc @@ -153,4 +153,33 @@ TEST_F(GenericFamilyTest, RenameBinary) { EXPECT_THAT(Run({"get", kKey2}), RespEq("bar")); } +using testing::Each; +using testing::StartsWith; +using testing::AnyOf; + +TEST_F(GenericFamilyTest, Scan) { + for (unsigned i = 0; i < 10; ++i) + Run({"set", absl::StrCat("key", i), "bar"}); + + for (unsigned i = 0; i < 10; ++i) + Run({"set", absl::StrCat("str", i), "bar"}); + + for (unsigned i = 0; i < 10; ++i) + Run({"sadd", absl::StrCat("set", i), "bar"}); + + for (unsigned i = 0; i < 10; ++i) + Run({"zadd", absl::StrCat("zset", i), "0", "bar"}); + + auto resp = Run({"scan", "0", "count", "20", "type", "string"}); + EXPECT_EQ(2, resp.size()); + auto vec = StrArray(resp[1]); + EXPECT_GT(vec.size(), 10); + EXPECT_THAT(vec, Each(AnyOf(StartsWith("str"), StartsWith("key")))); + + resp = Run({"scan", "0", "count", "20", "match", "zset*"}); + vec = StrArray(resp[1]); + EXPECT_EQ(10, vec.size()); + EXPECT_THAT(vec, Each(StartsWith("zset"))); +} + } // namespace dfly diff --git a/src/server/list_family.cc b/src/server/list_family.cc index 187e22e00..23666426b 100644 --- a/src/server/list_family.cc +++ b/src/server/list_family.cc @@ -380,23 +380,24 @@ void ListFamily::BPopGeneric(ListDir dir, CmdArgList args, ConnectionContext* cn BPopper popper(dir); OpStatus result = popper.Run(transaction, unsigned(timeout * 1000)); + if (result == OpStatus::OK) { + CHECK(popper.found()); + VLOG(1) << "BLPop returned "; + + auto res = popper.result(); + std::string_view str_arr[2] = {res.first, res.second}; + return (*cntx)->SendStringArr(str_arr); + } + switch (result) { case OpStatus::WRONG_TYPE: return (*cntx)->SendError(kWrongTypeErr); - case OpStatus::OK: - break; case OpStatus::TIMED_OUT: return (*cntx)->SendNullArray(); default: - LOG(FATAL) << "Unexpected error " << result; + LOG(ERROR) << "Unexpected error " << result; } - - CHECK(popper.found()); - VLOG(1) << "BLPop returned "; - - auto res = popper.result(); - std::string_view str_arr[2] = {res.first, res.second}; - return (*cntx)->SendStringArr(str_arr); + return (*cntx)->SendNullArray(); } void ListFamily::PushGeneric(ListDir dir, bool skip_notexists, CmdArgList args, diff --git a/src/server/rdb_load.cc b/src/server/rdb_load.cc index 0bb08201d..cd6746370 100644 --- a/src/server/rdb_load.cc +++ b/src/server/rdb_load.cc @@ -332,7 +332,8 @@ error_code RdbLoader::Load(io::Source* src) { } if (type == RDB_OPCODE_MODULE_AUX) { - return RdbError(errc::module_not_supported); + LOG(ERROR) << "Modules are not supported"; + return RdbError(errc::feature_not_supported); } if (!rdbIsObjectType(type)) { @@ -416,7 +417,8 @@ auto RdbLoader::LoadLen(bool* is_encoded) -> io::Result { res = absl::big_endian::Load64(mem_buf_.InputBuffer().data()); mem_buf_.ConsumeInput(8); } else { - LOG(FATAL) << "Unknown length encoding " << type << " in rdbLoadLen()"; + LOG(ERROR) << "Bad length encoding " << type << " in rdbLoadLen()"; + return Unexpected(errc::rdb_file_corrupted); } return res; @@ -501,7 +503,8 @@ error_code RdbLoader::HandleAux() { } else if (!strcasecmp((char*)auxkey->ptr, "repl-offset")) { // TODO } else if (!strcasecmp((char*)auxkey->ptr, "lua")) { - LOG(FATAL) << "Lua scripts are not supported"; + LOG(ERROR) << "Lua scripts are not supported"; + return RdbError(errc::feature_not_supported); } else if (!strcasecmp((char*)auxkey->ptr, "redis-ver")) { LOG(INFO) << "Loading RDB produced by version " << (char*)auxval->ptr; } else if (!strcasecmp((char*)auxkey->ptr, "ctime")) { @@ -575,7 +578,8 @@ auto RdbLoader::FetchGenericString(int flags) -> io::Result { case RDB_ENC_LZF: return FetchLzfStringObject(flags); default: - LOG(FATAL) << "Unknown RDB string encoding type " << len; + LOG(ERROR) << "Unknown RDB string encoding len " << len; + return Unexpected(errc::rdb_file_corrupted); } } diff --git a/src/server/rdb_save.cc b/src/server/rdb_save.cc index 0b3794b53..54af35458 100644 --- a/src/server/rdb_save.cc +++ b/src/server/rdb_save.cc @@ -216,8 +216,8 @@ error_code RdbSerializer::SaveObject(const PrimeValue& pv) { return SaveZSetObject(pv.AsRObj()); } - LOG(FATAL) << "Not implemented " << obj_type; - return error_code{}; + LOG(ERROR) << "Not implemented " << obj_type; + return make_error_code(errc::function_not_supported); } error_code RdbSerializer::SaveListObject(const robj* obj) { diff --git a/src/server/rdb_test.cc b/src/server/rdb_test.cc index 4164b8cff..ae1ed6e89 100644 --- a/src/server/rdb_test.cc +++ b/src/server/rdb_test.cc @@ -83,7 +83,7 @@ TEST_F(RdbTest, LoadSmall6) { auto ec = loader.Load(&fs); CHECK(!ec); auto resp = Run({"scan", "0"}); - EXPECT_THAT(Array(resp[1]), + EXPECT_THAT(StrArray(resp[1]), UnorderedElementsAre("list1", "hset_zl", "list2", "zset_sl", "intset", "set1", "zset_zl", "hset_ht", "intkey", "strkey")); resp = Run({"smembers", "intset"}); @@ -125,4 +125,11 @@ TEST_F(RdbTest, Reload) { EXPECT_EQ(2, CheckedInt({"ZCARD", "zs2"})); } +TEST_F(RdbTest, ReloadTtl) { + Run({"set", "key", "val"}); + Run({"expire", "key", "1000"}); + Run({"debug", "reload"}); + EXPECT_LT(990, CheckedInt({"ttl", "key"})); +} + } // namespace dfly diff --git a/src/server/test_utils.cc b/src/server/test_utils.cc index 5866f6e9c..b6e11c4c7 100644 --- a/src/server/test_utils.cc +++ b/src/server/test_utils.cc @@ -287,13 +287,18 @@ auto BaseFamilyTest::AddFindConn(Protocol proto, std::string_view id) -> TestCon return it->second.get(); } -RespVec BaseFamilyTest::Array(const RespExpr& expr) { +vector BaseFamilyTest::StrArray(const RespExpr& expr) { CHECK(expr.type == RespExpr::ARRAY || expr.type == RespExpr::NIL_ARRAY); if (expr.type == RespExpr::NIL_ARRAY) - return RespVec{}; + return vector{}; const RespVec* src = get(expr.u); - return *src; + vector res(src->size()); + for (size_t i = 0; i < src->size(); ++i) { + res[i] = ToSV(src->at(i).GetBuf()); + } + + return res; } } // namespace dfly diff --git a/src/server/test_utils.h b/src/server/test_utils.h index 5fa0e2814..4f3205c0e 100644 --- a/src/server/test_utils.h +++ b/src/server/test_utils.h @@ -66,7 +66,7 @@ class BaseFamilyTest : public ::testing::Test { } TestConnWrapper* AddFindConn(Protocol proto, std::string_view id); - static RespVec Array(const RespExpr& expr); + static std::vector StrArray(const RespExpr& expr); // ts is ms void UpdateTime(uint64_t ms); diff --git a/src/server/zset_family.cc b/src/server/zset_family.cc index 8d31d1f15..899eaf9ca 100644 --- a/src/server/zset_family.cc +++ b/src/server/zset_family.cc @@ -193,7 +193,8 @@ void IntervalVisitor::ActionRange(unsigned start, unsigned end) { Next(zl, &eptr, &sptr); } - } else if (zobj_->encoding == OBJ_ENCODING_SKIPLIST) { + } else { + CHECK_EQ(zobj_->encoding, OBJ_ENCODING_SKIPLIST); zset* zs = (zset*)zobj_->ptr; zskiplist* zsl = zs->zsl; zskiplistNode* ln; @@ -216,18 +217,15 @@ void IntervalVisitor::ActionRange(unsigned start, unsigned end) { result_.emplace_back(string(ele, sdslen(ele)), ln->score); ln = params_.reverse ? ln->backward : ln->level[0].forward; } - } else { - LOG(FATAL) << "Unknown sorted set encoding" << zobj_->encoding; } } void IntervalVisitor::ActionRange(const zrangespec& range) { if (zobj_->encoding == OBJ_ENCODING_LISTPACK) { ExtractListPack(range); - } else if (zobj_->encoding == OBJ_ENCODING_SKIPLIST) { - ExtractSkipList(range); } else { - LOG(FATAL) << "Unknown sorted set encoding " << zobj_->encoding; + CHECK_EQ(zobj_->encoding, OBJ_ENCODING_SKIPLIST); + ExtractSkipList(range); } } @@ -238,11 +236,10 @@ void IntervalVisitor::ActionRem(unsigned start, unsigned end) { removed_ = (end - start) + 1; zl = lpDeleteRange(zl, 2 * start, 2 * removed_); zobj_->ptr = zl; - } else if (zobj_->encoding == OBJ_ENCODING_SKIPLIST) { + } else { + CHECK_EQ(OBJ_ENCODING_SKIPLIST, zobj_->encoding); zset* zs = (zset*)zobj_->ptr; removed_ = zslDeleteRangeByRank(zs->zsl, start + 1, end + 1, zs->dict); - } else { - LOG(FATAL) << "Unknown sorted set encoding" << zobj_->encoding; } } @@ -253,11 +250,10 @@ void IntervalVisitor::ActionRem(const zrangespec& range) { zl = zzlDeleteRangeByScore(zl, &range, &deleted); zobj_->ptr = zl; removed_ = deleted; - } else if (zobj_->encoding == OBJ_ENCODING_SKIPLIST) { + } else { + CHECK_EQ(OBJ_ENCODING_SKIPLIST, zobj_->encoding); zset* zs = (zset*)zobj_->ptr; removed_ = zslDeleteRangeByScore(zs->zsl, &range, zs->dict); - } else { - LOG(FATAL) << "Unknown sorted set encoding" << zobj_->encoding; } }