Add nodiscard to methods of redis::Database (#1752)

Co-authored-by: hulk <hulk.website@gmail.com>

src/cluster/cluster.cc

@@ -127,7 +127,10 @@ Status Cluster::SetSlotRanges(const std::vector<SlotRange> &slot_ranges, const s
       if (old_node == myself_ && old_node != to_assign_node) {
         // If slot is migrated from this node
         if (migrated_slots_.count(slot) > 0) {
-          svr_->slot_migrator->ClearKeysOfSlot(kDefaultNamespace, slot);
+          auto s = svr_->slot_migrator->ClearKeysOfSlot(kDefaultNamespace, slot);
+          if (!s.ok()) {
+            LOG(ERROR) << "failed to clear data of migrated slot: " << s.ToString();
+          }
           migrated_slots_.erase(slot);
         }
         // If slot is imported into this node
@@ -209,7 +212,10 @@ Status Cluster::SetClusterNodes(const std::string &nodes_str, int64_t version, b
   if (!migrated_slots_.empty()) {
     for (auto &it : migrated_slots_) {
       if (slots_nodes_[it.first] != myself_) {
-        svr_->slot_migrator->ClearKeysOfSlot(kDefaultNamespace, it.first);
+        auto s = svr_->slot_migrator->ClearKeysOfSlot(kDefaultNamespace, it.first);
+        if (!s.ok()) {
+          LOG(ERROR) << "failed to clear data of migrated slots: " << s.ToString();
+        }
       }
     }
   }

src/commands/cmd_key.cc

@@ -125,7 +125,8 @@ class CommandExists : public Commander {
 
     int cnt = 0;
     redis::Database redis(svr->storage, conn->GetNamespace());
-    redis.Exists(keys, &cnt);
+    auto s = redis.Exists(keys, &cnt);
+    if (!s.ok()) return {Status::RedisExecErr, s.ToString()};
     *output = redis::Integer(cnt);
 
     return Status::OK();

src/commands/cmd_server.cc

@@ -136,14 +136,19 @@ class CommandKeys : public Commander {
     std::string prefix = args_[1];
     std::vector<std::string> keys;
     redis::Database redis(svr->storage, conn->GetNamespace());
+
+    rocksdb::Status s;
     if (prefix == "*") {
-      redis.Keys(std::string(), &keys);
+      s = redis.Keys(std::string(), &keys);
     } else {
       if (prefix[prefix.size() - 1] != '*') {
         return {Status::RedisExecErr, "only keys prefix match was supported"};
       }
 
-      redis.Keys(prefix.substr(0, prefix.size() - 1), &keys);
+      s = redis.Keys(prefix.substr(0, prefix.size() - 1), &keys);
+    }
+    if (!s.ok()) {
+      return {Status::RedisExecErr, s.ToString()};
     }
     *output = redis::MultiBulkString(keys);
     return Status::OK();
@@ -802,7 +807,10 @@ class CommandRandomKey : public Commander {
     std::string key;
     auto cursor = svr->GetLastRandomKeyCursor();
     redis::Database redis(svr->storage, conn->GetNamespace());
-    redis.RandomKey(cursor, &key);
+    auto s = redis.RandomKey(cursor, &key);
+    if (!s.ok()) {
+      return {Status::RedisExecErr, s.ToString()};
+    }
     svr->SetLastRandomKeyCursor(key);
     *output = redis::BulkString(key);
     return Status::OK();

src/commands/cmd_string.cc

@@ -296,14 +296,17 @@ class CommandSet : public Commander {
   Status Execute(Server *svr, Connection *conn, std::string *output) override {
     bool ret = false;
     redis::String string_db(svr->storage, conn->GetNamespace());
-    rocksdb::Status s;
 
     if (ttl_ < 0) {
-      string_db.Del(args_[1]);
+      auto s = string_db.Del(args_[1]);
+      if (!s.ok()) {
+        return {Status::RedisExecErr, s.ToString()};
+      }
       *output = redis::SimpleString("OK");
       return Status::OK();
     }
 
+    rocksdb::Status s;
     if (set_flag_ == NX) {
       s = string_db.SetNX(args_[1], args_[2], ttl_, &ret);
     } else if (set_flag_ == XX) {

src/server/server.cc

@@ -1304,8 +1304,12 @@ Status Server::AsyncScanDBSize(const std::string &ns) {
 
   return task_runner_.TryPublish([ns, this] {
     redis::Database db(storage, ns);
+
     KeyNumStats stats;
-    db.GetKeyNumStats("", &stats);
+    auto s = db.GetKeyNumStats("", &stats);
+    if (!s.ok()) {
+      LOG(ERROR) << "failed to retrieve key num stats: " << s.ToString();
+    }
 
     std::lock_guard<std::mutex> lg(db_job_mu_);
 

src/storage/redis_db.cc

@@ -75,8 +75,7 @@ rocksdb::Status Database::GetRawMetadata(const Slice &ns_key, std::string *bytes
 }
 
 rocksdb::Status Database::GetRawMetadataByUserKey(const Slice &user_key, std::string *bytes) {
-  std::string ns_key = AppendNamespacePrefix(user_key);
-  return GetRawMetadata(ns_key, bytes);
+  return GetRawMetadata(AppendNamespacePrefix(user_key), bytes);
 }
 
 rocksdb::Status Database::Expire(const Slice &user_key, uint64_t timestamp) {
@@ -479,7 +478,7 @@ rocksdb::Status Database::Dump(const Slice &user_key, std::vector<std::string> *
 
   if (metadata.Type() == kRedisList) {
     ListMetadata list_metadata(false);
-    GetMetadata(kRedisList, ns_key, &list_metadata);
+    s = GetMetadata(kRedisList, ns_key, &list_metadata);
     if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
     infos->emplace_back("head");
     infos->emplace_back(std::to_string(list_metadata.head));

src/storage/redis_db.h

@@ -34,30 +34,31 @@ class Database {
   static constexpr uint64_t RANDOM_KEY_SCAN_LIMIT = 60;
 
   explicit Database(engine::Storage *storage, std::string ns = "");
-  rocksdb::Status GetMetadata(RedisType type, const Slice &ns_key, Metadata *metadata);
-  rocksdb::Status GetRawMetadata(const Slice &ns_key, std::string *bytes);
-  rocksdb::Status GetRawMetadataByUserKey(const Slice &user_key, std::string *bytes);
-  rocksdb::Status Expire(const Slice &user_key, uint64_t timestamp);
-  rocksdb::Status Del(const Slice &user_key);
-  rocksdb::Status MDel(const std::vector<Slice> &keys, uint64_t *deleted_cnt);
-  rocksdb::Status Exists(const std::vector<Slice> &keys, int *ret);
-  rocksdb::Status TTL(const Slice &user_key, int64_t *ttl);
-  rocksdb::Status Type(const Slice &user_key, RedisType *type);
-  rocksdb::Status Dump(const Slice &user_key, std::vector<std::string> *infos);
-  rocksdb::Status FlushDB();
-  rocksdb::Status FlushAll();
-  rocksdb::Status GetKeyNumStats(const std::string &prefix, KeyNumStats *stats);
-  rocksdb::Status Keys(const std::string &prefix, std::vector<std::string> *keys = nullptr,
-                       KeyNumStats *stats = nullptr);
-  rocksdb::Status Scan(const std::string &cursor, uint64_t limit, const std::string &prefix,
-                       std::vector<std::string> *keys, std::string *end_cursor = nullptr);
-  rocksdb::Status RandomKey(const std::string &cursor, std::string *key);
+  [[nodiscard]] rocksdb::Status GetMetadata(RedisType type, const Slice &ns_key, Metadata *metadata);
+  [[nodiscard]] rocksdb::Status GetRawMetadata(const Slice &ns_key, std::string *bytes);
+  [[nodiscard]] rocksdb::Status GetRawMetadataByUserKey(const Slice &user_key, std::string *bytes);
+  [[nodiscard]] rocksdb::Status Expire(const Slice &user_key, uint64_t timestamp);
+  [[nodiscard]] rocksdb::Status Del(const Slice &user_key);
+  [[nodiscard]] rocksdb::Status MDel(const std::vector<Slice> &keys, uint64_t *deleted_cnt);
+  [[nodiscard]] rocksdb::Status Exists(const std::vector<Slice> &keys, int *ret);
+  [[nodiscard]] rocksdb::Status TTL(const Slice &user_key, int64_t *ttl);
+  [[nodiscard]] rocksdb::Status Type(const Slice &user_key, RedisType *type);
+  [[nodiscard]] rocksdb::Status Dump(const Slice &user_key, std::vector<std::string> *infos);
+  [[nodiscard]] rocksdb::Status FlushDB();
+  [[nodiscard]] rocksdb::Status FlushAll();
+  [[nodiscard]] rocksdb::Status GetKeyNumStats(const std::string &prefix, KeyNumStats *stats);
+  [[nodiscard]] rocksdb::Status Keys(const std::string &prefix, std::vector<std::string> *keys = nullptr,
+                                     KeyNumStats *stats = nullptr);
+  [[nodiscard]] rocksdb::Status Scan(const std::string &cursor, uint64_t limit, const std::string &prefix,
+                                     std::vector<std::string> *keys, std::string *end_cursor = nullptr);
+  [[nodiscard]] rocksdb::Status RandomKey(const std::string &cursor, std::string *key);
   std::string AppendNamespacePrefix(const Slice &user_key);
-  rocksdb::Status FindKeyRangeWithPrefix(const std::string &prefix, const std::string &prefix_end, std::string *begin,
-                                         std::string *end, rocksdb::ColumnFamilyHandle *cf_handle = nullptr);
-  rocksdb::Status ClearKeysOfSlot(const rocksdb::Slice &ns, int slot);
-  rocksdb::Status GetSlotKeysInfo(int slot, std::map<int, uint64_t> *slotskeys, std::vector<std::string> *keys,
-                                  int count);
+  [[nodiscard]] rocksdb::Status FindKeyRangeWithPrefix(const std::string &prefix, const std::string &prefix_end,
+                                                       std::string *begin, std::string *end,
+                                                       rocksdb::ColumnFamilyHandle *cf_handle = nullptr);
+  [[nodiscard]] rocksdb::Status ClearKeysOfSlot(const rocksdb::Slice &ns, int slot);
+  [[nodiscard]] rocksdb::Status GetSlotKeysInfo(int slot, std::map<int, uint64_t> *slotskeys,
+                                                std::vector<std::string> *keys, int count);
 
  protected:
   engine::Storage *storage_;

src/types/redis_geo.cc

@@ -149,7 +149,8 @@ rocksdb::Status Geo::SearchStore(const Slice &user_key, GeoShape geo_shape, Orig
     auto result_length = static_cast<int64_t>(geo_points->size());
     int64_t returned_items_count = (count == 0 || result_length < count) ? result_length : count;
     if (returned_items_count == 0) {
-      ZSet::Del(user_key);
+      auto s = ZSet::Del(user_key);
+      if (!s.ok()) return s;
     } else {
       std::vector<MemberScore> member_scores;
       for (const auto &geo_point : *geo_points) {

src/types/redis_string.cc

@@ -238,7 +238,8 @@ rocksdb::Status String::SetXX(const std::string &user_key, const std::string &va
 
   std::string ns_key = AppendNamespacePrefix(user_key);
   LockGuard guard(storage_->GetLockManager(), ns_key);
-  Exists({user_key}, &exists);
+  auto s = Exists({user_key}, &exists);
+  if (!s.ok()) return s;
   if (exists != 1) return rocksdb::Status::OK();
 
   *flag = true;

tests/cppunit/compact_test.cc

@@ -44,7 +44,7 @@ TEST(Compact, Filter) {
   std::string live_hash_key = "live_hash_key";
   hash->Set(expired_hash_key, "f1", "v1", &ret);
   hash->Set(expired_hash_key, "f2", "v2", &ret);
-  hash->Expire(expired_hash_key, 1);  // expired
+  auto st = hash->Expire(expired_hash_key, 1);  // expired
   usleep(10000);
   hash->Set(live_hash_key, "f1", "v1", &ret);
   hash->Set(live_hash_key, "f2", "v2", &ret);
@@ -75,7 +75,7 @@ TEST(Compact, Filter) {
   std::string expired_zset_key = "expire_zset_key";
   std::vector<MemberScore> member_scores = {MemberScore{"z1", 1.1}, MemberScore{"z2", 0.4}};
   zset->Add(expired_zset_key, ZAddFlags::Default(), &member_scores, &ret);
-  zset->Expire(expired_zset_key, 1);  // expired
+  st = zset->Expire(expired_zset_key, 1);  // expired
   usleep(10000);
 
   status = storage->Compact(nullptr, nullptr);

tests/cppunit/disk_test.cc

@@ -54,7 +54,7 @@ TEST_F(RedisDiskTest, StringDisk) {
   EXPECT_TRUE(disk->GetKeySize(key_, kRedisString, &result).ok());
   EXPECT_GE(result, value_size[0] * estimation_factor_);
   EXPECT_LE(result, value_size[0] / estimation_factor_);
-  string->Del(key_);
+  auto s = string->Del(key_);
 }
 
 TEST_F(RedisDiskTest, HashDisk) {
@@ -78,7 +78,7 @@ TEST_F(RedisDiskTest, HashDisk) {
   EXPECT_TRUE(disk->GetKeySize(key_, kRedisHash, &key_size).ok());
   EXPECT_GE(key_size, approximate_size * estimation_factor_);
   EXPECT_LE(key_size, approximate_size / estimation_factor_);
-  hash->Del(key_);
+  auto s = hash->Del(key_);
 }
 
 TEST_F(RedisDiskTest, SetDisk) {
@@ -103,7 +103,7 @@ TEST_F(RedisDiskTest, SetDisk) {
   EXPECT_GE(key_size, approximate_size * estimation_factor_);
   EXPECT_LE(key_size, approximate_size / estimation_factor_);
 
-  set->Del(key_);
+  s = set->Del(key_);
 }
 
 TEST_F(RedisDiskTest, ListDisk) {
@@ -126,7 +126,7 @@ TEST_F(RedisDiskTest, ListDisk) {
   EXPECT_TRUE(disk->GetKeySize(key_, kRedisList, &key_size).ok());
   EXPECT_GE(key_size, approximate_size * estimation_factor_);
   EXPECT_LE(key_size, approximate_size / estimation_factor_);
-  list->Del(key_);
+  s = list->Del(key_);
 }
 
 TEST_F(RedisDiskTest, ZsetDisk) {
@@ -148,7 +148,7 @@ TEST_F(RedisDiskTest, ZsetDisk) {
   EXPECT_TRUE(disk->GetKeySize(key_, kRedisZSet, &key_size).ok());
   EXPECT_GE(key_size, approximate_size * estimation_factor_);
   EXPECT_LE(key_size, approximate_size / estimation_factor_);
-  zset->Del(key_);
+  s = zset->Del(key_);
 }
 
 TEST_F(RedisDiskTest, BitmapDisk) {
@@ -165,7 +165,7 @@ TEST_F(RedisDiskTest, BitmapDisk) {
   EXPECT_TRUE(disk->GetKeySize(key_, kRedisBitmap, &key_size).ok());
   EXPECT_GE(key_size, approximate_size * estimation_factor_);
   EXPECT_LE(key_size, approximate_size / estimation_factor_);
-  bitmap->Del(key_);
+  auto s = bitmap->Del(key_);
 }
 
 TEST_F(RedisDiskTest, BitmapDisk2) {
@@ -175,7 +175,7 @@ TEST_F(RedisDiskTest, BitmapDisk2) {
       std::unique_ptr<redis::Bitmap> bitmap = std::make_unique<redis::Bitmap>(storage_, "disk_ns_bitmap2");
       std::unique_ptr<redis::Disk> disk = std::make_unique<redis::Disk>(storage_, "disk_ns_bitmap2");
       key_ = "bitmapdisk_key2";
-      bitmap->Del(key_);
+      auto s = bitmap->Del(key_);
       bool bit = false;
 
       for (size_t i = 0; i < num_bits; i += kGroupSize) {
@@ -222,7 +222,7 @@ TEST_F(RedisDiskTest, SortedintDisk) {
   EXPECT_TRUE(disk->GetKeySize(key_, kRedisSortedint, &key_size).ok());
   EXPECT_GE(key_size, approximate_size * estimation_factor_);
   EXPECT_LE(key_size, approximate_size / estimation_factor_);
-  sortedint->Del(key_);
+  auto s = sortedint->Del(key_);
 }
 
 TEST_F(RedisDiskTest, StreamDisk) {
@@ -243,5 +243,5 @@ TEST_F(RedisDiskTest, StreamDisk) {
   EXPECT_TRUE(disk->GetKeySize(key_, kRedisStream, &key_size).ok());
   EXPECT_GE(key_size, approximate_size * estimation_factor_);
   EXPECT_LE(key_size, approximate_size / estimation_factor_);
-  stream->Del(key_);
-}
+  auto s = stream->Del(key_);
+}

tests/cppunit/metadata_test.cc

@@ -90,7 +90,7 @@ TEST_F(RedisTypeTest, GetMetadata) {
   EXPECT_TRUE(s.ok() && fvs.size() == ret);
   HashMetadata metadata;
   std::string ns_key = redis_->AppendNamespacePrefix(key_);
-  redis_->GetMetadata(kRedisHash, ns_key, &metadata);
+  s = redis_->GetMetadata(kRedisHash, ns_key, &metadata);
   EXPECT_EQ(fvs.size(), metadata.size);
   s = redis_->Del(key_);
   EXPECT_TRUE(s.ok());
@@ -106,12 +106,12 @@ TEST_F(RedisTypeTest, Expire) {
   EXPECT_TRUE(s.ok() && fvs.size() == ret);
   int64_t now = 0;
   rocksdb::Env::Default()->GetCurrentTime(&now);
-  redis_->Expire(key_, now * 1000 + 2000);
+  s = redis_->Expire(key_, now * 1000 + 2000);
   int64_t ttl = 0;
-  redis_->TTL(key_, &ttl);
+  s = redis_->TTL(key_, &ttl);
   ASSERT_GT(ttl, 0);
   ASSERT_LE(ttl, 2000);
-  redis_->Del(key_);
+  s = redis_->Del(key_);
 }
 
 TEST(Metadata, MetadataDecodingBackwardCompatibleSimpleKey) {

tests/cppunit/types/bitmap_test.cc

@@ -46,7 +46,7 @@ TEST_F(RedisBitmapTest, GetAndSetBit) {
     bitmap_->GetBit(key_, offset, &bit);
     EXPECT_TRUE(bit);
   }
-  bitmap_->Del(key_);
+  auto s = bitmap_->Del(key_);
 }
 
 TEST_F(RedisBitmapTest, BitCount) {
@@ -60,7 +60,7 @@ TEST_F(RedisBitmapTest, BitCount) {
   EXPECT_EQ(cnt, 6);
   bitmap_->BitCount(key_, 0, -1, &cnt);
   EXPECT_EQ(cnt, 6);
-  bitmap_->Del(key_);
+  auto s = bitmap_->Del(key_);
 }
 
 TEST_F(RedisBitmapTest, BitPosClearBit) {
@@ -72,7 +72,7 @@ TEST_F(RedisBitmapTest, BitPosClearBit) {
     bitmap_->SetBit(key_, i, true, &old_bit);
     EXPECT_FALSE(old_bit);
   }
-  bitmap_->Del(key_);
+  auto s = bitmap_->Del(key_);
 }
 
 TEST_F(RedisBitmapTest, BitPosSetBit) {
@@ -87,5 +87,5 @@ TEST_F(RedisBitmapTest, BitPosSetBit) {
     bitmap_->BitPos(key_, true, start_indexes[i], -1, true, &pos);
     EXPECT_EQ(pos, offsets[i]);
   }
-  bitmap_->Del(key_);
+  auto s = bitmap_->Del(key_);
 }

tests/cppunit/types/bloom_chain_test.cc

@@ -49,15 +49,15 @@ TEST_F(RedisBloomChainTest, Reserve) {
   EXPECT_FALSE(s.ok());
   EXPECT_EQ(s.ToString(), "Invalid argument: the key already exists");
 
-  sb_chain_->Del(key_);
+  s = sb_chain_->Del(key_);
 }
 
 TEST_F(RedisBloomChainTest, BasicAddAndTest) {
   int ret = 0;
 
   auto s = sb_chain_->Exist("no_exist_key", "test_item", &ret);
   EXPECT_EQ(ret, 0);
-  sb_chain_->Del("no_exist_key");
+  s = sb_chain_->Del("no_exist_key");
 
   std::string insert_items[] = {"item1", "item2", "item3", "item101", "item202", "303"};
   for (const auto& insert_item : insert_items) {
@@ -78,5 +78,5 @@ TEST_F(RedisBloomChainTest, BasicAddAndTest) {
     EXPECT_TRUE(s.ok());
     EXPECT_EQ(ret, 0);
   }
-  sb_chain_->Del(key_);
+  s = sb_chain_->Del(key_);
 }