git-hulk commented on code in PR #2402:
URL: https://github.com/apache/kvrocks/pull/2402#discussion_r1682115639
##########
src/storage/redis_metadata.h:
##########
@@ -200,9 +200,22 @@ class Metadata {
static uint64_t generateVersion();
};
+enum class HashSubkeyEncoding : uint8_t {
+ RAW = 0,
Review Comment:
RAW => VALUE_ONLY,
WITH_TTL => VALUE_WITH_TTL
##########
src/types/redis_hash.cc:
##########
@@ -418,4 +472,208 @@ rocksdb::Status Hash::RandField(const Slice &user_key,
int64_t command_count, st
return rocksdb::Status::OK();
}
+rocksdb::Status Hash::ExpireFields(const Slice &user_key, uint64_t expire_ms,
const std::vector<Slice> &fields,
+ HashFieldExpireType type, bool is_persist,
std::vector<int8_t> *ret) {
+ std::string ns_key = AppendNamespacePrefix(user_key);
+ HashMetadata metadata(false);
+ LatestSnapShot ss(storage_);
+ rocksdb::Status s = GetMetadata(GetOptions{ss.GetSnapShot()}, ns_key,
&metadata);
+ if (!s.ok() && !s.IsNotFound()) return s;
+ if (s.IsNotFound()) {
+ ret->resize(fields.size(), -2);
+ return rocksdb::Status::OK();
+ }
+
+ rocksdb::ReadOptions read_options = storage_->DefaultMultiGetOptions();
+ read_options.snapshot = ss.GetSnapShot();
+
+ std::vector<rocksdb::Slice> keys;
+ keys.reserve(fields.size());
+ std::vector<std::string> sub_keys;
+ sub_keys.resize(fields.size());
+ for (size_t i = 0; i < fields.size(); i++) {
+ auto &field = fields[i];
+ sub_keys[i] = InternalKey(ns_key, field, metadata.version,
storage_->IsSlotIdEncoded()).Encode();
+ keys.emplace_back(sub_keys[i]);
+ }
+
+ auto batch = storage_->GetWriteBatchBase();
+ WriteBatchLogData log_data(kRedisHash);
+ batch->PutLogData(log_data.Encode());
+
+ // expire special field
+ std::vector<rocksdb::PinnableSlice> values_vector;
Review Comment:
Don't bring the type into the variable naming, values_vector should be
`values`, same as the `statuses`
##########
src/types/redis_hash.cc:
##########
@@ -418,4 +472,208 @@ rocksdb::Status Hash::RandField(const Slice &user_key,
int64_t command_count, st
return rocksdb::Status::OK();
}
+rocksdb::Status Hash::ExpireFields(const Slice &user_key, uint64_t expire_ms,
const std::vector<Slice> &fields,
+ HashFieldExpireType type, bool is_persist,
std::vector<int8_t> *ret) {
+ std::string ns_key = AppendNamespacePrefix(user_key);
Review Comment:
Need to add the lock guard to prevent data race for the user key.
##########
src/types/redis_hash.cc:
##########
@@ -418,4 +472,208 @@ rocksdb::Status Hash::RandField(const Slice &user_key,
int64_t command_count, st
return rocksdb::Status::OK();
}
+rocksdb::Status Hash::ExpireFields(const Slice &user_key, uint64_t expire_ms,
const std::vector<Slice> &fields,
Review Comment:
I think we should distinguish this in Commands instead of DB implementation.
##########
src/storage/compact_filter.cc:
##########
@@ -131,6 +132,17 @@ bool SubKeyFilter::Filter(int level, const Slice &key,
const Slice &value, std::
return false;
}
+ if (metadata.Type() == kRedisHash) {
+ HashMetadata hash_metadata(false);
+ Slice bytes(cached_metadata_);
+ if (!hash_metadata.Decode(bytes).ok()) {
+ LOG(ERROR) << "[compact_filter/subkey] Failed to get hash_metadata"
+ << ", namespace: " << ikey.GetNamespace() << ", key: " <<
ikey.GetKey() << ", err: " << s.Msg();
+ return false;
+ }
+ return redis::Hash::IsFieldExpired(hash_metadata, value);
+ }
+
return IsMetadataExpired(ikey, metadata) || (metadata.Type() == kRedisBitmap
&& redis::Bitmap::IsEmptySegment(value));
Review Comment:
Can check if Metadata is expired first
##########
src/types/redis_hash.h:
##########
@@ -63,9 +65,16 @@ class Hash : public SubKeyScanner {
std::vector<std::string> *values = nullptr);
rocksdb::Status RandField(const Slice &user_key, int64_t command_count,
std::vector<FieldValue> *field_values,
HashFetchType type = HashFetchType::kOnlyKey);
+ rocksdb::Status ExpireFields(const Slice &user_key, uint64_t expire_ms,
const std::vector<Slice> &fields,
+ HashFieldExpireType type, bool is_persist,
std::vector<int8_t> *ret);
+ rocksdb::Status TTLFields(const Slice &user_key, const std::vector<Slice>
&fields, std::vector<int64_t> *ret);
+ static bool IsExpiredField(Metadata &metadata, const Slice &value);
private:
rocksdb::Status GetMetadata(Database::GetOptions get_options, const Slice
&ns_key, HashMetadata *metadata);
+ static rocksdb::Status decodeFieldValue(const HashMetadata &metadata,
std::string *value, uint64_t &expire);
+ static rocksdb::Status encodeValueExpire(std::string *value, uint64_t
expire);
+ static bool isMeetCondition(HashFieldExpireType type, uint64_t new_expire,
uint64_t old_expire);
Review Comment:
Agree with what @torwig mentioned, for:
`decodeFieldAndTTL` should be separated into two methods:
`decodeExpireFromValue` and `checkIfFieldExpired`, and `encodeFieldAndTTL` can
rename to `encodeExpireToValue`. @torwig What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
