This is an automated email from the ASF dual-hosted git repository.
twice pushed a commit to branch unstable
in repository https://gitbox.apache.org/repos/asf/kvrocks.git
The following commit(s) were added to refs/heads/unstable by this push:
new b9c7359e Add nodiscard to Metadata::Decode (#1751)
b9c7359e is described below
commit b9c7359eb12c4f08ccd8547cd3132173405faf7d
Author: Twice <[email protected]>
AuthorDate: Sun Sep 10 16:58:14 2023 +0900
Add nodiscard to Metadata::Decode (#1751)
---
src/cluster/slot_migrate.cc | 8 +++++--
src/common/db_util.h | 7 +++---
src/storage/batch_extractor.cc | 3 ++-
src/storage/redis_db.cc | 51 +++++++++++++++++++++++++++++-------------
src/storage/redis_db.h | 5 +++--
src/storage/redis_metadata.h | 4 ++--
src/types/redis_bitmap.cc | 10 +++++----
src/types/redis_string.cc | 10 +++++++--
tests/cppunit/metadata_test.cc | 12 +++++-----
utils/kvrocks2redis/parser.cc | 8 +++++--
10 files changed, 78 insertions(+), 40 deletions(-)
diff --git a/src/cluster/slot_migrate.cc b/src/cluster/slot_migrate.cc
index 307c426f..900be3b1 100644
--- a/src/cluster/slot_migrate.cc
+++ b/src/cluster/slot_migrate.cc
@@ -584,7 +584,9 @@ StatusOr<KeyMigrationResult>
SlotMigrator::migrateOneKey(const rocksdb::Slice &k
std::string
*restore_cmds) {
std::string bytes = encoded_metadata.ToString();
Metadata metadata(kRedisNone, false);
- metadata.Decode(bytes);
+ if (auto s = metadata.Decode(bytes); !s.ok()) {
+ return {Status::NotOK, s.ToString()};
+ }
if (metadata.Type() != kRedisString && metadata.Type() != kRedisStream &&
metadata.size == 0) {
return KeyMigrationResult::kUnderlyingStructEmpty;
@@ -617,7 +619,9 @@ StatusOr<KeyMigrationResult>
SlotMigrator::migrateOneKey(const rocksdb::Slice &k
}
case kRedisStream: {
StreamMetadata stream_md(false);
- stream_md.Decode(bytes);
+ if (auto s = stream_md.Decode(bytes); !s.ok()) {
+ return {Status::NotOK, s.ToString()};
+ }
auto s = migrateStream(key, stream_md, restore_cmds);
if (!s.IsOK()) {
diff --git a/src/common/db_util.h b/src/common/db_util.h
index 788c9685..a7379c61 100644
--- a/src/common/db_util.h
+++ b/src/common/db_util.h
@@ -55,9 +55,10 @@ StatusOr<std::unique_ptr<T>> WrapOutPtrToUnique(Args&&...
args) {
return ptr;
}
-inline rocksdb::Status DBOpenForReadOnly(const rocksdb::DBOptions& db_options,
const std::string& dbname,
- const
std::vector<rocksdb::ColumnFamilyDescriptor>& column_families,
-
std::vector<rocksdb::ColumnFamilyHandle*>* handles, rocksdb::DB** dbptr) {
+[[nodiscard]] inline rocksdb::Status DBOpenForReadOnly(
+ const rocksdb::DBOptions& db_options, const std::string& dbname,
+ const std::vector<rocksdb::ColumnFamilyDescriptor>& column_families,
+ std::vector<rocksdb::ColumnFamilyHandle*>* handles, rocksdb::DB** dbptr) {
return rocksdb::DB::OpenForReadOnly(db_options, dbname, column_families,
handles, dbptr);
}
diff --git a/src/storage/batch_extractor.cc b/src/storage/batch_extractor.cc
index 7a5804f9..c66aca80 100644
--- a/src/storage/batch_extractor.cc
+++ b/src/storage/batch_extractor.cc
@@ -58,7 +58,8 @@ rocksdb::Status WriteBatchExtractor::PutCF(uint32_t
column_family_id, const Slic
}
Metadata metadata(kRedisNone);
- metadata.Decode(value);
+ auto s = metadata.Decode(value);
+ if (!s.ok()) return s;
if (metadata.Type() == kRedisString) {
command_args = {"SET", user_key,
value.ToString().substr(Metadata::GetOffsetAfterExpire(value[0]))};
diff --git a/src/storage/redis_db.cc b/src/storage/redis_db.cc
index f0fcea44..76aecaf7 100644
--- a/src/storage/redis_db.cc
+++ b/src/storage/redis_db.cc
@@ -44,21 +44,25 @@ rocksdb::Status Database::GetMetadata(RedisType type, const
Slice &ns_key, Metad
std::string bytes;
auto s = GetRawMetadata(ns_key, &bytes);
if (!s.ok()) return s;
- metadata->Decode(bytes);
+ s = metadata->Decode(bytes);
+ if (!s.ok()) return s;
if (metadata->Expired()) {
- metadata->Decode(old_metadata);
+ // error discarded here since it already failed
+ auto _ [[maybe_unused]] = metadata->Decode(old_metadata);
return rocksdb::Status::NotFound(kErrMsgKeyExpired);
}
if (metadata->Type() != type &&
(metadata->size > 0 || metadata->Type() == kRedisString ||
metadata->Type() == kRedisStream)) {
- metadata->Decode(old_metadata);
+ // error discarded here since it already failed
+ auto _ [[maybe_unused]] = metadata->Decode(old_metadata);
return rocksdb::Status::InvalidArgument(kErrMsgWrongType);
}
if (metadata->size == 0 && type != kRedisStream &&
type != kRedisBloomFilter) { // stream and bloom is allowed to be empty
- metadata->Decode(old_metadata);
- return rocksdb::Status::NotFound("no elements");
+ // error discarded here since it already failed
+ auto _ [[maybe_unused]] = metadata->Decode(old_metadata);
+ return rocksdb::Status::NotFound("no element found");
}
return s;
}
@@ -83,7 +87,9 @@ rocksdb::Status Database::Expire(const Slice &user_key,
uint64_t timestamp) {
LockGuard guard(storage_->GetLockManager(), ns_key);
rocksdb::Status s = storage_->Get(rocksdb::ReadOptions(),
metadata_cf_handle_, ns_key, &value);
if (!s.ok()) return s;
- metadata.Decode(value);
+
+ s = metadata.Decode(value);
+ if (!s.ok()) return s;
if (metadata.Expired()) {
return rocksdb::Status::NotFound(kErrMsgKeyExpired);
}
@@ -114,7 +120,8 @@ rocksdb::Status Database::Del(const Slice &user_key) {
rocksdb::Status s = storage_->Get(rocksdb::ReadOptions(),
metadata_cf_handle_, ns_key, &value);
if (!s.ok()) return s;
Metadata metadata(kRedisNone, false);
- metadata.Decode(value);
+ s = metadata.Decode(value);
+ if (!s.ok()) return s;
if (metadata.Expired()) {
return rocksdb::Status::NotFound(kErrMsgKeyExpired);
}
@@ -155,7 +162,8 @@ rocksdb::Status Database::MDel(const std::vector<Slice>
&keys, uint64_t *deleted
if (statuses[i].IsNotFound()) continue;
Metadata metadata(kRedisNone, false);
- metadata.Decode(pin_values[i]);
+ auto s = metadata.Decode(pin_values[i]);
+ if (!s.ok()) continue;
if (metadata.Expired()) continue;
batch->Delete(metadata_cf_handle_, lock_keys[i]);
@@ -181,7 +189,8 @@ rocksdb::Status Database::Exists(const std::vector<Slice>
&keys, int *ret) {
if (!s.ok() && !s.IsNotFound()) return s;
if (s.ok()) {
Metadata metadata(kRedisNone, false);
- metadata.Decode(value);
+ s = metadata.Decode(value);
+ if (!s.ok()) return s;
if (!metadata.Expired()) *ret += 1;
}
}
@@ -200,15 +209,18 @@ rocksdb::Status Database::TTL(const Slice &user_key,
int64_t *ttl) {
if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
Metadata metadata(kRedisNone, false);
- metadata.Decode(value);
+ s = metadata.Decode(value);
+ if (!s.ok()) return s;
*ttl = metadata.TTL();
return rocksdb::Status::OK();
}
-void Database::GetKeyNumStats(const std::string &prefix, KeyNumStats *stats) {
Keys(prefix, nullptr, stats); }
+rocksdb::Status Database::GetKeyNumStats(const std::string &prefix,
KeyNumStats *stats) {
+ return Keys(prefix, nullptr, stats);
+}
-void Database::Keys(const std::string &prefix, std::vector<std::string> *keys,
KeyNumStats *stats) {
+rocksdb::Status Database::Keys(const std::string &prefix,
std::vector<std::string> *keys, KeyNumStats *stats) {
uint16_t slot_id = 0;
std::string ns_prefix;
if (namespace_ != kDefaultNamespace || keys != nullptr) {
@@ -236,7 +248,8 @@ void Database::Keys(const std::string &prefix,
std::vector<std::string> *keys, K
break;
}
Metadata metadata(kRedisNone, false);
- metadata.Decode(iter->value());
+ auto s = metadata.Decode(iter->value());
+ if (!s.ok()) continue;
if (metadata.Expired()) {
if (stats) stats->n_expired++;
continue;
@@ -267,6 +280,8 @@ void Database::Keys(const std::string &prefix,
std::vector<std::string> *keys, K
if (stats && stats->n_expires > 0) {
stats->avg_ttl = ttl_sum / stats->n_expires / 1000;
}
+
+ return rocksdb::Status::OK();
}
rocksdb::Status Database::Scan(const std::string &cursor, uint64_t limit,
const std::string &prefix,
@@ -312,7 +327,9 @@ rocksdb::Status Database::Scan(const std::string &cursor,
uint64_t limit, const
break;
}
Metadata metadata(kRedisNone, false);
- metadata.Decode(iter->value());
+ 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());
keys->emplace_back(user_key);
@@ -432,7 +449,8 @@ rocksdb::Status Database::Dump(const Slice &user_key,
std::vector<std::string> *
if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
Metadata metadata(kRedisNone, false);
- metadata.Decode(value);
+ s = metadata.Decode(value);
+ if (!s.ok()) return s;
infos->emplace_back("namespace");
infos->emplace_back(namespace_);
@@ -484,7 +502,8 @@ rocksdb::Status Database::Type(const Slice &user_key,
RedisType *type) {
if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
Metadata metadata(kRedisNone, false);
- metadata.Decode(value);
+ s = metadata.Decode(value);
+ if (!s.ok()) return s;
if (metadata.Expired()) {
*type = kRedisNone;
} else {
diff --git a/src/storage/redis_db.h b/src/storage/redis_db.h
index 60bf0129..625e3d71 100644
--- a/src/storage/redis_db.h
+++ b/src/storage/redis_db.h
@@ -46,8 +46,9 @@ class Database {
rocksdb::Status Dump(const Slice &user_key, std::vector<std::string> *infos);
rocksdb::Status FlushDB();
rocksdb::Status FlushAll();
- void GetKeyNumStats(const std::string &prefix, KeyNumStats *stats);
- void Keys(const std::string &prefix, std::vector<std::string> *keys =
nullptr, KeyNumStats *stats = nullptr);
+ 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);
diff --git a/src/storage/redis_metadata.h b/src/storage/redis_metadata.h
index 015379e6..0512d1f9 100644
--- a/src/storage/redis_metadata.h
+++ b/src/storage/redis_metadata.h
@@ -148,8 +148,8 @@ class Metadata {
bool ExpireAt(uint64_t expired_ts) const;
virtual void Encode(std::string *dst) const;
- virtual rocksdb::Status Decode(Slice *input);
- rocksdb::Status Decode(Slice input);
+ [[nodiscard]] virtual rocksdb::Status Decode(Slice *input);
+ [[nodiscard]] rocksdb::Status Decode(Slice input);
bool operator==(const Metadata &that) const;
virtual ~Metadata() = default;
diff --git a/src/types/redis_bitmap.cc b/src/types/redis_bitmap.cc
index 71318d02..a7b73847 100644
--- a/src/types/redis_bitmap.cc
+++ b/src/types/redis_bitmap.cc
@@ -43,19 +43,21 @@ rocksdb::Status Bitmap::GetMetadata(const Slice &ns_key,
BitmapMetadata *metadat
metadata->Encode(&old_metadata);
auto s = GetRawMetadata(ns_key, raw_value);
if (!s.ok()) return s;
- metadata->Decode(*raw_value);
+ s = metadata->Decode(*raw_value);
+ if (!s.ok()) return s;
if (metadata->Expired()) {
- metadata->Decode(old_metadata);
+ // error discarded here since it already failed
+ auto _ [[maybe_unused]] = metadata->Decode(old_metadata);
return rocksdb::Status::NotFound(kErrMsgKeyExpired);
}
if (metadata->Type() == kRedisString) return s;
if (metadata->Type() != kRedisBitmap && metadata->size > 0) {
- metadata->Decode(old_metadata);
+ auto _ [[maybe_unused]] = metadata->Decode(old_metadata);
return rocksdb::Status::InvalidArgument(kErrMsgWrongType);
}
if (metadata->size == 0) {
- metadata->Decode(old_metadata);
+ auto _ [[maybe_unused]] = metadata->Decode(old_metadata);
return rocksdb::Status::NotFound("no elements");
}
return s;
diff --git a/src/types/redis_string.cc b/src/types/redis_string.cc
index ab07d329..26211444 100644
--- a/src/types/redis_string.cc
+++ b/src/types/redis_string.cc
@@ -48,7 +48,12 @@ std::vector<rocksdb::Status> String::getRawValues(const
std::vector<Slice> &keys
if (!statuses[i].ok()) continue;
(*raw_values)[i].assign(pin_values[i].data(), pin_values[i].size());
Metadata metadata(kRedisNone, false);
- metadata.Decode((*raw_values)[i]);
+ auto s = metadata.Decode((*raw_values)[i]);
+ if (!s.ok()) {
+ (*raw_values)[i].clear();
+ statuses[i] = s;
+ continue;
+ }
if (metadata.Expired()) {
(*raw_values)[i].clear();
statuses[i] = rocksdb::Status::NotFound(kErrMsgKeyExpired);
@@ -73,7 +78,8 @@ rocksdb::Status String::getRawValue(const std::string
&ns_key, std::string *raw_
if (!s.ok()) return s;
Metadata metadata(kRedisNone, false);
- metadata.Decode(*raw_value);
+ s = metadata.Decode(*raw_value);
+ if (!s.ok()) return s;
if (metadata.Expired()) {
raw_value->clear();
return rocksdb::Status::NotFound(kErrMsgKeyExpired);
diff --git a/tests/cppunit/metadata_test.cc b/tests/cppunit/metadata_test.cc
index e5b2963e..8a8b5fe4 100644
--- a/tests/cppunit/metadata_test.cc
+++ b/tests/cppunit/metadata_test.cc
@@ -48,7 +48,7 @@ TEST(Metadata, EncodeAndDecode) {
string_md.expire = 123000;
string_md.Encode(&string_bytes);
Metadata string_md1(kRedisNone);
- string_md1.Decode(string_bytes);
+ ASSERT_TRUE(string_md1.Decode(string_bytes).ok());
ASSERT_EQ(string_md, string_md1);
ListMetadata list_md;
list_md.flags = 13;
@@ -60,7 +60,7 @@ TEST(Metadata, EncodeAndDecode) {
ListMetadata list_md1;
std::string list_bytes;
list_md.Encode(&list_bytes);
- list_md1.Decode(list_bytes);
+ ASSERT_TRUE(list_md1.Decode(list_bytes).ok());
ASSERT_EQ(list_md, list_md1);
}
@@ -124,7 +124,7 @@ TEST(Metadata, MetadataDecodingBackwardCompatibleSimpleKey)
{
EXPECT_EQ(encoded_bytes.size(), 5);
Metadata md_new(kRedisNone, false, true); // decoding existing metadata
with 64-bit feature activated
- md_new.Decode(encoded_bytes);
+ ASSERT_TRUE(md_new.Decode(encoded_bytes).ok());
EXPECT_FALSE(md_new.Is64BitEncoded());
EXPECT_EQ(md_new.Type(), kRedisString);
EXPECT_EQ(md_new.expire, expire_at);
@@ -151,7 +151,7 @@ TEST(Metadata,
MetadataDecodingBackwardCompatibleComplexKey) {
md_old.Encode(&encoded_bytes);
Metadata md_new(kRedisHash, false, true);
- md_new.Decode(encoded_bytes);
+ ASSERT_TRUE(md_new.Decode(encoded_bytes).ok());
EXPECT_FALSE(md_new.Is64BitEncoded());
EXPECT_EQ(md_new.Type(), kRedisHash);
EXPECT_EQ(md_new.expire, expire_at);
@@ -167,7 +167,7 @@ TEST(Metadata, Metadata64bitExpiration) {
md_src.Encode(&encoded_bytes);
Metadata md_decoded(kRedisNone, false, true);
- md_decoded.Decode(encoded_bytes);
+ ASSERT_TRUE(md_decoded.Decode(encoded_bytes).ok());
EXPECT_TRUE(md_decoded.Is64BitEncoded());
EXPECT_EQ(md_decoded.Type(), kRedisString);
EXPECT_EQ(md_decoded.expire, expire_at);
@@ -182,7 +182,7 @@ TEST(Metadata, Metadata64bitSize) {
md_src.Encode(&encoded_bytes);
Metadata md_decoded(kRedisNone, false, true);
- md_decoded.Decode(encoded_bytes);
+ ASSERT_TRUE(md_decoded.Decode(encoded_bytes).ok());
EXPECT_TRUE(md_decoded.Is64BitEncoded());
EXPECT_EQ(md_decoded.Type(), kRedisHash);
EXPECT_EQ(md_decoded.size, big_size);
diff --git a/utils/kvrocks2redis/parser.cc b/utils/kvrocks2redis/parser.cc
index 9d96f156..0c463d9a 100644
--- a/utils/kvrocks2redis/parser.cc
+++ b/utils/kvrocks2redis/parser.cc
@@ -40,15 +40,19 @@ Status Parser::ParseFullDB() {
read_options.snapshot = latest_snapshot_->GetSnapShot();
read_options.fill_cache = false;
std::unique_ptr<rocksdb::Iterator> iter(db->NewIterator(read_options,
metadata_cf_handle));
- Status s;
for (iter->SeekToFirst(); iter->Valid(); iter->Next()) {
Metadata metadata(kRedisNone);
- metadata.Decode(iter->value());
+ auto ds = metadata.Decode(iter->value());
+ if (!ds.ok()) {
+ continue;
+ }
+
if (metadata.Expired()) { // ignore the expired key
continue;
}
+ Status s;
if (metadata.Type() == kRedisString) {
s = parseSimpleKV(iter->key(), iter->value(), metadata.expire);
} else {
