Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add nodiscard to Metadata::Decode #1751

Merged
merged 2 commits into from
Sep 10, 2023
Merged

Add nodiscard to Metadata::Decode #1751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c0fadd6
Add nodiscard to Metadata::Decode
PragmaTwice Sep 9, 2023
5b934b1
Merge branch 'unstable' into nodiscard-metadata
PragmaTwice Sep 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/cluster/slot_migrate.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()) {
Expand Down
7 changes: 4 additions & 3 deletions src/common/db_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
3 changes: 2 additions & 1 deletion src/storage/batch_extractor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]))};
Expand Down
51 changes: 35 additions & 16 deletions src/storage/redis_db.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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]);
Expand All @@ -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;
}
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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_);
Expand Down Expand Up @@ -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 {
Expand Down
5 changes: 3 additions & 2 deletions src/storage/redis_db.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions src/storage/redis_metadata.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
10 changes: 6 additions & 4 deletions src/types/redis_bitmap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
10 changes: 8 additions & 2 deletions src/types/redis_string.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down
12 changes: 6 additions & 6 deletions tests/cppunit/metadata_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
}

Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down
8 changes: 6 additions & 2 deletions utils/kvrocks2redis/parser.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down