fix: duplicate meta key read #2744
Conversation
wangshao1
requested review from
AlexStocks,
cheniujh,
dingxiaoshuai123 and
Mixficsol
WalkthroughThe changes introduce a Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Outside diff range and nitpick comments (1)
src/storage/src/redis_streams.cc (1)
Line range hint
178-187: Consider handling the potential exception fromstd::movewhenprefetch_metais empty.The use of
std::move(prefetch_meta)directly in the function call toGetStreamMetaon line 187 could lead to unexpected behavior ifprefetch_metais empty or not properly initialized. Consider adding a check or ensuring thatprefetch_metais always valid when passed to this function.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- src/storage/src/redis.h (7 hunks)
- src/storage/src/redis_hashes.cc (6 hunks)
- src/storage/src/redis_lists.cc (6 hunks)
- src/storage/src/redis_sets.cc (6 hunks)
- src/storage/src/redis_streams.cc (4 hunks)
- src/storage/src/redis_strings.cc (15 hunks)
- src/storage/src/redis_zsets.cc (6 hunks)
- src/storage/src/storage.cc (1 hunks)
Additional comments not posted (11)
src/storage/src/redis.h (3)
120-149: The addition of thestd::string&& prefetch_metaparameter to the expiration, deletion, and TTL methods is a significant change aimed at improving metadata handling efficiency. Ensure that all callers of these methods are updated to pass this new parameter correctly.Verification successful
All instances of the modified methods in
src/storage/src/redis.hhave been updated to pass the new parameterstd::string&& prefetch_metacorrectly.
src/storage/src/redis.hsrc/storage/src/redis_zsets.ccsrc/storage/src/redis_streams.ccsrc/storage/src/redis_strings.ccsrc/storage/src/redis_sets.ccsrc/storage/src/redis_lists.ccsrc/storage/src/redis_hashes.ccsrc/storage/src/storage.ccScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to modified methods match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type cpp $'StringsExpire|HashesExpire|ListsExpire|ZsetsExpire|SetsExpire|StringsDel|HashesDel|ListsDel|ZsetsDel|SetsDel|StreamsDel|StringsExpireat|HashesExpireat|ListsExpireat|SetsExpireat|ZsetsExpireat|StringsPersist|HashesPersist|ListsPersist|ZsetsPersist|SetsPersist|StringsTTL|HashesTTL|ListsTTL|ZsetsTTL|SetsTTL'Length of output: 8999
326-326: Updates to theXRangeandGetStreamMetamethods to include astd::string&& prefetch_metaparameter are crucial for stream processing efficiency. It's important to validate that these changes integrate seamlessly with existing stream-related functionalities and that they contribute positively to performance metrics.Also applies to: 336-336
272-272: TheLLenmethod now includes astd::string&& prefetch_metaparameter. Verify that this parameter is being used effectively within the method and that its addition does not introduce any performance or functionality issues.Verification successful
The implementation of the
LLenmethod insrc/storage/src/redis_lists.ccshows that theprefetch_metaparameter is effectively utilized. Specifically, it is moved intometa_valueand used to determine if additional database operations are necessary. This approach ensures that ifprefetch_metais provided, redundant database reads are avoided, which could potentially improve performance.
src/storage/src/redis_lists.cc: TheLLenmethod uses theprefetch_meta` parameter effectively.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check all calls to `LLen` for correct usage of the new signature. rg --type cpp 'LLen'Length of output: 2567
Script:
#!/bin/bash # Extract the implementation of the `LLen` method from `src/storage/src/redis_lists.cc`. ast-grep --lang cpp --pattern $'Status Redis::LLen(const Slice& key, uint64_t* len, std::string&& prefetch_meta) { $$$ }' src/storage/src/redis_lists.ccLength of output: 155
Script:
#!/bin/bash # Search for the `LLen` method definition and implementation across the entire repository. rg 'Status Redis::LLen' --type cppLength of output: 150
Script:
#!/bin/bash # Extract the full implementation of the `LLen` method from `src/storage/src/redis_lists.cc`. sed -n '/Status Redis::LLen(const Slice& key, uint64_t* len, std::string&& prefetch_meta)/,/^}/p' src/storage/src/redis_lists.ccLength of output: 130
Script:
#!/bin/bash # Extract the implementation of the `LLen` method from `src/storage/src/redis_lists.cc`. ast-grep --lang cpp --pattern $'function_definition name: "LLen" body: $_ ' src/storage/src/redis_lists.ccLength of output: 454
Script:
#!/bin/bash # Search for the `LLen` method implementation in `src/storage/src/redis_lists.cc` and surrounding lines. rg -A 20 -B 5 'Status Redis::LLen' src/storage/src/redis_lists.ccLength of output: 1078
src/storage/src/redis_lists.cc (2)
1113-1126: Review timestamp handling inListsExpireat.
[REFACTOR_SUGGESTion]- if (timestamp > 0) { + // Set a new expiration time if a positive timestamp is provided + if (timestamp > 0 && timestamp > GetCurrentTime()) { parsed_lists_meta_value.SetEtime(static_cast<uint64_t>(timestamp)); return db_->Put(default_write_options_, handles_[kMetaCF], base_meta_key.Encode(), meta_value); } else { parsed_lists_meta_value.InitialMetaValue(); return db_->Put(default_write_options_, handles_[kMetaCF], base_meta_key.Encode(), meta_value); }Ensure that the timestamp is in the future before setting it as the new expiration time to avoid logical errors.
1081-1094: Check the deletion logic inListsDel.Verify through additional unit tests or integration tests that the deletion logic behaves as expected when
prefetch_metais provided and when it is not.src/storage/src/redis_sets.cc (1)
1370-1383: Check for potential data races or concurrency issues.The method
SetsDelmodifies the database state based on possibly stale metadata. Ensure that there are proper locking mechanisms in place to handle concurrent modifications to the same key.Verification successful
The search results show that the
SetsDelmethod insrc/storage/src/redis_sets.ccdoes use aScopeRecordLockto handle concurrent access to the key. This indicates that there are proper locking mechanisms in place to manage concurrent modifications to the same key.
src/storage/src/redis_sets.cc:1372: TheScopeRecordLock l(lock_mgr_, key);line ensures that the key is locked during the execution ofSetsDel.Given this evidence, the necessary locking mechanisms appear to be implemented correctly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify locking mechanisms for the SetsDel operation # Test: Search for lock usage around SetsDel calls. Expect: Proper locking mechanisms. rg --type cpp $'ScopeRecordLock l(lock_mgr_, key);\\n.*\\n.*SetsDel'Length of output: 259
Script:
#!/bin/bash # Description: Verify locking mechanisms for the SetsDel operation # Test: Search for lock usage around SetsDel calls. Expect: Proper locking mechanisms. rg --multiline --type cpp $'ScopeRecordLock l(lock_mgr_, key);\n.*\n.*SetsDel'Length of output: 77
Script:
#!/bin/bash # Description: Verify locking mechanisms for the SetsDel operation # Test: Search for SetsDel calls and examine surrounding code for locking mechanisms. rg --multiline --type cpp $'SetsDel\(.*\)' -A 5 -B 5Length of output: 3075
src/storage/src/redis_zsets.cc (5)
1598-1611: Refactor duplicate error handling code.
[REFACTOR_SUGGESTion]- if (s.ok() && !ExpectedMetaValue(DataType::kZSets, meta_value)) { - if (ExpectedStale(meta_value)) { - s = Status::NotFound(); - } else { - return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kZSets)] + "get type: " + DataTypeStrings[static_cast[int](GetMetaValueType(meta_value))); - } - } + if (!HandleMetaValueCheck(s, meta_value, DataType::kZSets, key)) { + return s; + }Consider extracting the repeated pattern of checking
meta_valueand handling errors into a helper functionHandleMetaValueCheckto reduce code duplication and improve maintainability.Also applies to: 1611-1611
1632-1645: Refactor duplicate error handling code.
[REFACTOR_SUGGESTion]- if (s.ok() && !ExpectedMetaValue(DataType::kZSets, meta_value)) { - if (ExpectedStale(meta_value)) { - s = Status::NotFound(); - } else { - return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast[int](DataType::kZSets)] + "get type: " + DataTypeStrings[static_cast[int](GetMetaValueType(meta_value))); - } - } + if (!HandleMetaValueCheck(s, meta_value, DataType::kZSets, key)) { + return s; + }Same suggestion as above to extract the repeated pattern of checking
meta_valueand handling errors into a helper functionHandleMetaValueCheckto reduce code duplication and improve maintainability.Also applies to: 1645-1645
1664-1677: Refactor duplicate error handling code.
[REFACTOR_SUGGESTion]- if (s.ok() && !ExpectedMetaValue(DataType::kZSets, meta_value)) { - if (ExpectedStale(meta_value)) { - s = Status::NotFound(); - } else { - return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast[int](DataType::kZSets)] + "get type: " + DataTypeStrings[static_cast[int](GetMetaValueType(meta_value))); - } - } + if (!HandleMetaValueCheck(s, meta_value, DataType::kZSets, key)) { + return s; + }Same suggestion as above to extract the repeated pattern of checking
meta_valueand handling errors into a helper functionHandleMetaValueCheckto reduce code duplication and improve maintainability.Also applies to: 1677-1677
1781-1793: Refactor duplicate error handling code.
[REFACTOR_SUGGESTion]- if (s.ok() && !ExpectedMetaValue(DataType::kZSets, meta_value)) { - if (ExpectedStale(meta_value)) { - s = Status::NotFound(); - } else { - return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast[int](DataType::kZSets)] + "get type: " + DataTypeStrings[static_cast[int](GetMetaValueType(meta_value))); - } - } + if (!HandleMetaValueCheck(s, meta_value, DataType::kZSets, key)) { + return s; + }Same suggestion as above to extract the repeated pattern of checking
meta_valueand handling errors into a helper functionHandleMetaValueCheckto reduce code duplication and improve maintainability.Also applies to: 1793-1793
1815-1826: Refactor duplicate error handling code.
[REFACTOR_SUGGESTion]- if (s.ok() && !ExpectedMetaValue(DataType::kZSets, meta_value)) { - if (ExpectedStale(meta_value)) { - s = Status::NotFound(); - } else { - return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast[int](DataType::kZSets)] + "get type: " + DataTypeStrings[static_cast[int](GetMetaValueType(meta_value))); - } - } + if (!HandleMetaValueCheck(s, meta_value, DataType::kZSets, key)) { + return s; + }Same suggestion as above to extract the repeated pattern of checking
meta_valueand handling errors into a helper functionHandleMetaValueCheckto reduce code duplication and improve maintainability.Also applies to: 1826-1826
src/storage/src/redis_streams.cc
Outdated
| rocksdb::ReadOptions& read_options, std::string&& prefetch_meta) { | ||
| std::string value(std::move(prefetch_meta)); | ||
| BaseMetaKey base_meta_key(key); | ||
| auto s = db_->Get(read_options, handles_[kMetaCF], base_meta_key.Encode(), &value); | ||
| if (s.ok() && !ExpectedMetaValue(DataType::kStreams, value)) { | ||
| if (ExpectedStale(value)) { | ||
| s = Status::NotFound(); | ||
| } else { | ||
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kStreams)] + "get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(value))]); | ||
| Status s; | ||
| if (value.empty()) { | ||
| s = db_->Get(read_options, handles_[kMetaCF], base_meta_key.Encode(), &value); | ||
| if (s.ok() && !ExpectedMetaValue(DataType::kStreams, value)) { | ||
| if (ExpectedStale(value)) { | ||
| s = Status::NotFound(); | ||
| } else { | ||
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kStreams)] + "get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(value))]); | ||
| } |
There was a problem hiding this comment.
Ensure robust handling of empty prefetch_meta in GetStreamMeta.
Similar to the earlier comment in XRange, the handling of an empty prefetch_meta in GetStreamMeta might lead to issues if not managed correctly. Consider adding explicit checks or handling strategies to manage cases where prefetch_meta might be empty or invalid.
+ if (value.empty()) {
+ return Status::InvalidArgument("Prefetch meta data cannot be empty.");
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rocksdb::ReadOptions& read_options, std::string&& prefetch_meta) { | |
| std::string value(std::move(prefetch_meta)); | |
| BaseMetaKey base_meta_key(key); | |
| auto s = db_->Get(read_options, handles_[kMetaCF], base_meta_key.Encode(), &value); | |
| if (s.ok() && !ExpectedMetaValue(DataType::kStreams, value)) { | |
| if (ExpectedStale(value)) { | |
| s = Status::NotFound(); | |
| } else { | |
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kStreams)] + "get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(value))]); | |
| Status s; | |
| if (value.empty()) { | |
| s = db_->Get(read_options, handles_[kMetaCF], base_meta_key.Encode(), &value); | |
| if (s.ok() && !ExpectedMetaValue(DataType::kStreams, value)) { | |
| if (ExpectedStale(value)) { | |
| s = Status::NotFound(); | |
| } else { | |
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kStreams)] + "get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(value))]); | |
| } | |
| rocksdb::ReadOptions& read_options, std::string&& prefetch_meta) { | |
| std::string value(std::move(prefetch_meta)); | |
| if (value.empty()) { | |
| return Status::InvalidArgument("Prefetch meta data cannot be empty."); | |
| } | |
| BaseMetaKey base_meta_key(key); | |
| Status s; | |
| if (value.empty()) { | |
| s = db_->Get(read_options, handles_[kMetaCF], base_meta_key.Encode(), &value); | |
| if (s.ok() && !ExpectedMetaValue(DataType::kStreams, value)) { | |
| if (ExpectedStale(value)) { | |
| s = Status::NotFound(); | |
| } else { | |
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kStreams)] + "get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(value))]); | |
| } |
There was a problem hiding this comment.
这里的value为empty表明还没有执行过getmeta的操作，所以需要进行get。如果不是empty，表明已经执行过getmeta操作，而且类型一定是匹配的，此时只需要解析meta value即可。
There was a problem hiding this comment.
@wangshao1, thanks for clarifying the operational logic behind handling an empty prefetch_meta in the GetStreamMeta function. Your explanation about the necessity of performing a get operation when value is empty, and directly parsing the meta value when it's not, helps ensure the correct handling of metadata based on its state. I appreciate your insight and will note this for future reviews.
Learnings added
User: wangshao1
PR: OpenAtomFoundation/pika#2744
File: src/storage/src/redis_streams.cc:0-0
Timestamp: 2024-06-20T06:55:33.236Z
Learning: In the `GetStreamMeta` function of `redis_streams.cc`, an empty `prefetch_meta` indicates that the `getmeta` operation has not been performed, necessitating a database fetch. If `prefetch_meta` is not empty, it implies that the meta value has already been fetched and the type matches, allowing for direct parsing.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/storage/src/redis_hashes.cc (6 hunks)
- src/storage/src/redis_strings.cc (15 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/storage/src/redis_hashes.cc
- src/storage/src/redis_strings.cc
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/storage/src/redis_hashes.cc (22 hunks)
- src/storage/src/redis_lists.cc (22 hunks)
- src/storage/src/redis_sets.cc (29 hunks)
- src/storage/src/redis_streams.cc (4 hunks)
- src/storage/src/redis_strings.cc (34 hunks)
- src/storage/src/redis_zsets.cc (30 hunks)
Files skipped from review as they are similar to previous changes (5)
- src/storage/src/redis_lists.cc
- src/storage/src/redis_sets.cc
- src/storage/src/redis_streams.cc
- src/storage/src/redis_strings.cc
- src/storage/src/redis_zsets.cc
src/storage/src/redis_hashes.cc
Outdated
| Status Redis::HashesExpire(const Slice& key, int64_t ttl, std::string&& prefetch_meta) { | ||
| std::string meta_value(std::move(prefetch_meta)); | ||
| ScopeRecordLock l(lock_mgr_, key); | ||
|
|
||
| BaseMetaKey base_meta_key(key); | ||
| Status s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | ||
| if (s.ok() && !ExpectedMetaValue(DataType::kHashes, meta_value)) { | ||
| if (ExpectedStale(meta_value)) { | ||
| s = Status::NotFound(); | ||
| } else { | ||
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kHashes)] + "get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | ||
| Status s; | ||
|
|
||
| if (meta_value.empty()) { | ||
| Status s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | ||
| if (s.ok() && !ExpectedMetaValue(DataType::kHashes, meta_value)) { | ||
| if (ExpectedStale(meta_value)) { | ||
| s = Status::NotFound(); | ||
| } else { | ||
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kHashes)] + ", get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | ||
| } |
There was a problem hiding this comment.
Ensure proper handling of meta_value being empty after attempting to fetch from the database.
if (meta_value.empty()) {
Status s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value);
+ if (!s.ok()) {
+ return s; // Properly handle the case where the meta_value could not be fetched
+ }
if (!ExpectedMetaValue(DataType::kHashes, meta_value)) {
if (ExpectedStale(meta_value)) {
s = Status::NotFound();
} else {
return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kHashes)] + ", get type: " + DataTypeStrings[staticCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Status Redis::HashesExpire(const Slice& key, int64_t ttl, std::string&& prefetch_meta) { | |
| std::string meta_value(std::move(prefetch_meta)); | |
| ScopeRecordLock l(lock_mgr_, key); | |
| BaseMetaKey base_meta_key(key); | |
| Status s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | |
| if (s.ok() && !ExpectedMetaValue(DataType::kHashes, meta_value)) { | |
| if (ExpectedStale(meta_value)) { | |
| s = Status::NotFound(); | |
| } else { | |
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kHashes)] + "get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | |
| Status s; | |
| if (meta_value.empty()) { | |
| Status s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | |
| if (s.ok() && !ExpectedMetaValue(DataType::kHashes, meta_value)) { | |
| if (ExpectedStale(meta_value)) { | |
| s = Status::NotFound(); | |
| } else { | |
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kHashes)] + ", get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | |
| } | |
| if (meta_value.empty()) { | |
| Status s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | |
| if (!s.ok()) { | |
| return s; // Properly handle the case where the meta_value could not be fetched | |
| } | |
| if (!ExpectedMetaValue(DataType::kHashes, meta_value)) { | |
| if (ExpectedStale(meta_value)) { | |
| s = Status::NotFound(); | |
| } else { | |
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kHashes)] + ", get type: " + DataTypeStrings[static |
src/storage/src/redis_hashes.cc
Outdated
| Status Redis::HashesExpireat(const Slice& key, int64_t timestamp, std::string&& prefetch_meta) { | ||
| std::string meta_value(std::move(prefetch_meta)); | ||
| ScopeRecordLock l(lock_mgr_, key); | ||
|
|
||
| BaseMetaKey base_meta_key(key); | ||
| Status s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | ||
| if (s.ok() && !ExpectedMetaValue(DataType::kHashes, meta_value)) { | ||
| if (ExpectedStale(meta_value)) { | ||
| s = Status::NotFound(); | ||
| } else { | ||
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kHashes)] + "get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | ||
| Status s; | ||
| if (meta_value.empty()) { | ||
| s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | ||
| if (s.ok() && !ExpectedMetaValue(DataType::kHashes, meta_value)) { | ||
| if (ExpectedStale(meta_value)) { | ||
| s = Status::NotFound(); | ||
| } else { | ||
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kHashes)] + ", get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | ||
| } |
There was a problem hiding this comment.
Check for potential unhandled cases where meta_value remains empty after fetching.
if (meta_value.empty()) {
s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value);
+ if (!s.ok()) {
+ return s; // Ensure that errors during fetching are handled
+ }
if (!ExpectedMetaValue(DataType::kHashes, meta_value)) {
if (ExpectedStale(meta_value)) {
s = Status::NotFound();
} else {
return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kHashes)] + ", get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]);
}
}
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Status Redis::HashesExpireat(const Slice& key, int64_t timestamp, std::string&& prefetch_meta) { | |
| std::string meta_value(std::move(prefetch_meta)); | |
| ScopeRecordLock l(lock_mgr_, key); | |
| BaseMetaKey base_meta_key(key); | |
| Status s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | |
| if (s.ok() && !ExpectedMetaValue(DataType::kHashes, meta_value)) { | |
| if (ExpectedStale(meta_value)) { | |
| s = Status::NotFound(); | |
| } else { | |
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kHashes)] + "get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | |
| Status s; | |
| if (meta_value.empty()) { | |
| s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | |
| if (s.ok() && !ExpectedMetaValue(DataType::kHashes, meta_value)) { | |
| if (ExpectedStale(meta_value)) { | |
| s = Status::NotFound(); | |
| } else { | |
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kHashes)] + ", get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | |
| } | |
| if (meta_value.empty()) { | |
| s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | |
| if (!s.ok()) { | |
| return s; // Ensure that errors during fetching are handled | |
| } | |
| if (!ExpectedMetaValue(DataType::kHashes, meta_value)) { | |
| if (ExpectedStale(meta_value)) { | |
| s = Status::NotFound(); | |
| } else { | |
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kHashes)] + ", get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | |
| } | |
| } | |
| } |
src/storage/src/redis_hashes.cc
Outdated
| Status Redis::HashesTTL(const Slice& key, int64_t* timestamp, std::string&& prefetch_meta) { | ||
| std::string meta_value(std::move(prefetch_meta)); | ||
| Status s; | ||
| BaseMetaKey base_meta_key(key); | ||
| Status s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | ||
| if (s.ok() && !ExpectedMetaValue(DataType::kHashes, meta_value)) { | ||
| if (ExpectedStale(meta_value)) { | ||
| s = Status::NotFound(); | ||
| } else { | ||
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kHashes)] + "get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | ||
| if (meta_value.empty()) { | ||
| s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | ||
| if (s.ok() && !ExpectedMetaValue(DataType::kHashes, meta_value)) { | ||
| if (ExpectedStale(meta_value)) { | ||
| s = Status::NotFound(); | ||
| } else { | ||
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kHashes)] + ", get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | ||
| } |
There was a problem hiding this comment.
Ensure error handling is robust when meta_value is not set after a fetch.
if (meta_value.empty()) {
s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value);
+ if (!s.ok()) {
+ return s; // Ensure robust handling of cases where meta_value remains unset
+ }
if (!ExpectedMetaValue(DataType::kHashes, meta_value)) {
if (ExpectedStale(meta_value)) {
s = Status::NotFound();
} else {
return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int](DataType::kHashes)] + ", get type: " + DataTypeStrings[static_cast<int](GetMetaValueType(meta_value))]);
}
}
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Status Redis::HashesTTL(const Slice& key, int64_t* timestamp, std::string&& prefetch_meta) { | |
| std::string meta_value(std::move(prefetch_meta)); | |
| Status s; | |
| BaseMetaKey base_meta_key(key); | |
| Status s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | |
| if (s.ok() && !ExpectedMetaValue(DataType::kHashes, meta_value)) { | |
| if (ExpectedStale(meta_value)) { | |
| s = Status::NotFound(); | |
| } else { | |
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kHashes)] + "get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | |
| if (meta_value.empty()) { | |
| s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | |
| if (s.ok() && !ExpectedMetaValue(DataType::kHashes, meta_value)) { | |
| if (ExpectedStale(meta_value)) { | |
| s = Status::NotFound(); | |
| } else { | |
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kHashes)] + ", get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | |
| } | |
| Status Redis::HashesTTL(const Slice& key, int64_t* timestamp, std::string&& prefetch_meta) { | |
| std::string meta_value(std::move(prefetch_meta)); | |
| Status s; | |
| BaseMetaKey base_meta_key(key); | |
| if (meta_value.empty()) { | |
| s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | |
| if (!s.ok()) { | |
| return s; // Ensure robust handling of cases where meta_value remains unset | |
| } | |
| if (!ExpectedMetaValue(DataType::kHashes, meta_value)) { | |
| if (ExpectedStale(meta_value)) { | |
| s = Status::NotFound(); | |
| } else { | |
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kHashes)] + ", get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | |
| } | |
| } | |
| } |
AlexStocks
changed the title
There was a problem hiding this comment.
Actionable comments posted: 25
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- src/storage/src/redis_hashes.cc (22 hunks)
- src/storage/src/redis_lists.cc (24 hunks)
- src/storage/src/redis_sets.cc (29 hunks)
- src/storage/src/redis_streams.cc (4 hunks)
- src/storage/src/redis_strings.cc (35 hunks)
- src/storage/src/redis_zsets.cc (30 hunks)
- src/storage/src/storage.cc (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- src/storage/src/redis_hashes.cc
- src/storage/src/redis_sets.cc
- src/storage/src/redis_streams.cc
- src/storage/src/redis_zsets.cc
- src/storage/src/storage.cc
Additional context used
Learnings (1)
src/storage/src/redis_lists.cc (1)
User: wangshao1 PR: OpenAtomFoundation/pika#2744 File: src/storage/src/redis_streams.cc:0-0 Timestamp: 2024-06-20T06:55:33.400Z Learning: In the `GetStreamMeta` function of `redis_streams.cc`, an empty `prefetch_meta` indicates that the `getmeta` operation has not been performed, necessitating a database fetch. If `prefetch_meta` is not empty, it implies that the meta value has already been fetched and the type matches, allowing for direct parsing.
Additional comments not posted (2)
src/storage/src/redis_lists.cc (2)
220-238: The implementation ofLLennow uses aprefetch_metaparameter to potentially skip a metadata fetch. This is a good use of the parameter to optimize performance. However, ensure that the function's callers are updated to pass this new parameter.
1100-1118: The modifications to incorporateprefetch_metain expiration, deletion, and TTL handling functions are consistent with the PR's goal to optimize metadata handling. Ensure that these changes are well-documented and tested to avoid regressions.Also applies to: 1140-1158, 1177-1195, 1216-1234, 1256-1273
Verification successful
The repeated error suggests that the command structure or search pattern might be fundamentally flawed. To accurately verify if all related functions are updated to handle the new
prefetch_metaparameter, we should try a different approach to gather the necessary information.
The search results confirm that the functions
ListsExpire,ListsDel,ListsExpireat,ListsPersist, andListsTTLhave been updated to include the newprefetch_metaparameter in both their declarations and definitions.
src/storage/src/redis_strings.ccshows calls to these functions with the new parameter.src/storage/src/redis.handsrc/storage/src/redis_lists.ccshow the updated function signatures.This aligns with the review comment's goal to optimize metadata handling by incorporating
prefetch_meta.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all related functions are updated to handle the new `prefetch_meta` parameter. # Test: Search for function calls without the new parameter. Expect: No occurrences. rg --type cpp "ListsExpire\(|ListsDel\(|ListsExpireat\(|ListsPersist\(|ListsTTL\(" --files-without-match "std::string&& prefetch_meta"Length of output: 204
Script:
#!/bin/bash # Description: Verify that all related functions are updated to handle the new `prefetch_meta` parameter. # Test: Search for function definitions without the new parameter. Expect: No occurrences. rg --type cpp "ListsExpire\(|ListsDel\(|ListsExpireat\(|ListsPersist\(|ListsTTL\(" --files-without-match "std::string&& prefetch_meta"Length of output: 204
Script:
#!/bin/bash # Description: Verify that all related functions are updated to handle the new `prefetch_meta` parameter. # Test: Search for function definitions without the new parameter. Expect: No occurrences. rg --type cpp "ListsExpire\(|ListsDel\(|ListsExpireat\(|ListsPersist\(|ListsTTL\(" --files-without-match "std::string&& prefetch_meta"Length of output: 204
Script:
#!/bin/bash # Description: Verify that all related functions are updated to handle the new `prefetch_meta` parameter. # Test: Search for function definitions without the new parameter. Expect: No occurrences. # Correcting the search pattern to ensure accurate results rg --type cpp "ListsExpire\(|ListsDel\(|ListsExpireat\(|ListsPersist\(|ListsTTL\(" --files-without-match "std::string&& prefetch_meta"Length of output: 204
Script:
#!/bin/bash # Description: Verify that all related functions are updated to handle the new `prefetch_meta` parameter. # Test: Search for the function definitions and ensure that they include the new parameter. # Search for the function definitions of interest and print their signatures rg --type cpp "^(.*ListsExpire|.*ListsDel|.*ListsExpireat|.*ListsPersist|.*ListsTTL)\s*\(" -g '*.cc' -g '*.h'Length of output: 1732
| Status Redis::StringsPersist(const Slice& key, std::string&& prefetch_meta) { | ||
| std::string value(std::move(prefetch_meta)); | ||
| ScopeRecordLock l(lock_mgr_, key); | ||
|
|
||
| BaseKey base_key(key); | ||
| Status s = db_->Get(default_read_options_, base_key.Encode(), &value); | ||
| if (s.ok() && !ExpectedMetaValue(DataType::kStrings, value)) { | ||
| if (ExpectedStale(value)) { | ||
| s = Status::NotFound(); | ||
| } else { | ||
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kStrings)] + "get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(value))]); | ||
| Status s; | ||
|
|
||
| // value is empty means no meta value get before, | ||
| // we should get meta first | ||
| if (value.empty()) { | ||
| s = db_->Get(default_read_options_, base_key.Encode(), &value); | ||
| if (s.ok() && !ExpectedMetaValue(DataType::kStrings, value)) { | ||
| if (ExpectedStale(value)) { | ||
| s = Status::NotFound(); | ||
| } else { | ||
| return Status::InvalidArgument( | ||
| "WRONGTYPE, key: " + key.ToString() + ", expect type: " + | ||
| DataTypeStrings[static_cast<int>(DataType::kStrings)] + ", get type: " + | ||
| DataTypeStrings[static_cast<int>(GetMetaValueType(value))]); |
There was a problem hiding this comment.
Centralize error message creation to enhance code cleanliness.
Repeated logic for generating type error messages suggests the need for a centralized method. This would make the code cleaner and easier to maintain.
- "WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast[int](DataType::kStrings)] + ", get type: " + DataTypeStrings[static_cast[int](GetMetaValueType(value))]
+ return handleTypeError(key.ToString(), value);Committable suggestion was skipped due to low confidence.
| } | ||
| // when reaches here, either s is not found or s is ok but expired |
| return s; | ||
| } | ||
| if (s.ok() && !ExpectedStale(old_value)) { | ||
| return s; |
There was a problem hiding this comment.
这里返回了个 ok 回去， 需要让 *ret = 0吗?
wangshao1
force-pushed
the
bugfix/fix_repeat_get_meta
branch
from
9d1f589 to
017088a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- src/storage/src/redis.h (7 hunks)
- src/storage/src/redis_hashes.cc (22 hunks)
- src/storage/src/redis_lists.cc (24 hunks)
- src/storage/src/redis_sets.cc (29 hunks)
- src/storage/src/redis_streams.cc (4 hunks)
- src/storage/src/redis_strings.cc (35 hunks)
- src/storage/src/redis_zsets.cc (30 hunks)
- src/storage/src/storage.cc (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- src/storage/src/redis.h
- src/storage/src/redis_hashes.cc
- src/storage/src/redis_streams.cc
- src/storage/src/redis_zsets.cc
- src/storage/src/storage.cc
Additional context used
Learnings (3)
src/storage/src/redis_lists.cc (1)
User: wangshao1 PR: OpenAtomFoundation/pika#2744 File: src/storage/src/redis_streams.cc:0-0 Timestamp: 2024-06-20T06:55:33.400Z Learning: In the `GetStreamMeta` function of `redis_streams.cc`, an empty `prefetch_meta` indicates that the `getmeta` operation has not been performed, necessitating a database fetch. If `prefetch_meta` is not empty, it implies that the meta value has already been fetched and the type matches, allowing for direct parsing.src/storage/src/redis_strings.cc (2)
User: wangshao1 PR: OpenAtomFoundation/pika#2744 File: src/storage/src/redis_strings.cc:0-0 Timestamp: 2024-06-20T09:21:39.323Z Learning: Refactor involving extensive code changes should be deferred to subsequent versions to avoid risks before major releases. This is tracked under issue #2751 for the Pika project.User: wangshao1 PR: OpenAtomFoundation/pika#2744 File: src/storage/src/redis_sets.cc:0-0 Timestamp: 2024-06-20T09:25:58.037Z Learning: Refactor involving extensive code changes should be deferred to subsequent versions to avoid risks before major releases. This is tracked under issue #2751 for the Pika project.src/storage/src/redis_sets.cc (2)
User: wangshao1 PR: OpenAtomFoundation/pika#2744 File: src/storage/src/redis.h:249-249 Timestamp: 2024-06-20T09:24:32.373Z Learning: In certain parts of the codebase, the `SCard` function is called with an empty string for the `std::string&& prefetch_meta` parameter because there is no pre-fetched metadata. In these cases, the function is designed to fetch the metadata from the database if the `prefetch_meta` is empty.User: wangshao1 PR: OpenAtomFoundation/pika#2744 File: src/storage/src/redis_sets.cc:0-0 Timestamp: 2024-06-20T09:25:58.037Z Learning: Refactor involving extensive code changes should be deferred to subsequent versions to avoid risks before major releases. This is tracked under issue #2751 for the Pika project.
Additional comments not posted (23)
src/storage/src/redis_lists.cc (18)
270-270: Skip: Duplicate comment.This comment regarding error handling for incorrect data types was previously addressed in lines 220-238.
Also applies to: 271-271, 272-272, 273-273
328-328: Skip: Duplicate comment.This comment regarding error handling for incorrect data types was previously addressed in lines 220-238.
Also applies to: 329-329, 330-330, 331-331
384-384: Skip: Duplicate comment.This comment regarding error handling for incorrect data types was previously addressed in lines 220-238.
Also applies to: 385-385, 386-386, 387-387
428-428: Skip: Duplicate comment.This comment regarding error handling for incorrect data types was previously addressed in lines 220-238.
Also applies to: 429-429, 430-430, 431-431
488-488: Skip: Duplicate comment.This comment regarding error handling for incorrect data types was previously addressed in lines 220-238.
Also applies to: 489-489, 490-490, 491-491
558-558: Skip: Duplicate comment.This comment regarding error handling for incorrect data types was previously addressed in lines 220-238.
Also applies to: 559-559, 560-560, 561-561
690-690: Skip: Duplicate comment.This comment regarding error handling for incorrect data types was previously addressed in lines 220-238.
Also applies to: 691-691, 692-692, 693-693
734-734: Skip: Duplicate comment.This comment regarding error handling for incorrect data types was previously addressed in lines 220-238.
Also applies to: 735-735, 736-736, 737-737
807-807: Skip: Duplicate comment.This comment regarding error handling for incorrect data types was previously addressed in lines 220-238.
Also applies to: 808-808, 809-809, 810-810
864-864: Skip: Duplicate comment.This comment regarding error handling for incorrect data types was previously addressed in lines 220-238.
Also applies to: 865-865, 866-866, 867-867
957-957: Skip: Duplicate comment.This comment regarding error handling for incorrect data types was previously addressed in lines 220-238.
Also applies to: 958-958, 959-959, 960-960
1014-1014: Skip: Duplicate comment.This comment regarding error handling for incorrect data types was previously addressed in lines 220-238.
Also applies to: 1015-1015, 1016-1016, 1017-1017
1070-1070: Skip: Duplicate comment.This comment regarding error handling for incorrect data types was previously addressed in lines 220-238.
Also applies to: 1071-1071, 1072-1072, 1073-1073
1100-1100: Skip: Duplicate comment.This comment regarding error handling for incorrect data types was previously addressed in lines 220-238.
Also applies to: 1101-1101, 1104-1104, 1105-1105, 1106-1106, 1107-1107, 1108-1108, 1109-1109, 1110-1110, 1111-1111, 1112-1112, 1113-1113, 1114-1114, 1115-1115, 1116-1116, 1117-1117, 1118-1118
1140-1140: Skip: Duplicate comment.This comment regarding error handling for incorrect data types was previously addressed in lines 220-238.
Also applies to: 1141-1141, 1144-1144, 1145-1145, 1146-1146, 1147-1147, 1148-1148, 1149-1149, 1150-1150, 1151-1151, 1152-1152, 1153-1153, 1154-1154, 1155-1155, 1156-1156, 1157-1157, 1158-1158
1177-1177: Skip: Duplicate comment.This comment regarding error handling for incorrect data types was previously addressed in lines 220-238.
Also applies to: 1178-1178, 1181-1181, 1182-1182, 1183-1183, 1184-1184, 1185-1185, 1186-1186, 1187-1187, 1188-1188, 1189-1189, 1190-1190, 1191-1191, 1192-1192, 1193-1193, 1194-1194, 1195-1195
1216-1216: Skip: Duplicate comment.This comment regarding error handling for incorrect data types was previously addressed in lines 220-238.
Also applies to: 1217-1217, 1220-1220, 1221-1221, 1222-1222, 1223-1223, 1224-1224, 1225-1225, 1226-1226, 1227-1227, 1228-1228, 1229-1229, 1230-1230, 1231-1231, 1232-1232, 1233-1233, 1234-1234
1256-1256: Skip: Duplicate comment.This comment regarding error handling for incorrect data types was previously addressed in lines 220-238.
Also applies to: 1257-1257, 1259-1259, 1260-1260, 1261-1261, 1262-1262, 1263-1263, 1264-1264, 1265-1265, 1266-1266, 1267-1267, 1268-1268, 1269-1269, 1270-1270, 1271-1271, 1272-1272, 1273-1273
src/storage/src/redis_strings.cc (5)
1299-1316: Add prefetch_meta parameter to StringsExpire functionThe
StringsExpirefunction has been updated to include astd::string&& prefetch_metaparameter. This change allows for more efficient metadata handling by potentially reducing the need for a metadata fetch if it's already prefetched. Ensure that all calls to this function are updated to pass the new parameter where necessary.
1335-1352: Add prefetch_meta parameter to StringsDel functionThe
StringsDelfunction now accepts astd::string&& prefetch_metaparameter, which is intended to optimize metadata handling by using prefetched metadata. This update aligns with the changes made across other string-related functions, maintaining consistency in the API. Ensure that calls toStringsDelare updated accordingly.
1366-1383: Add prefetch_meta parameter to StringsExpireat functionThe function
StringsExpireathas been modified to include astd::string&& prefetch_metaparameter. This change is part of a broader effort to optimize metadata handling across the Redis implementation. It's important to ensure that all function calls are updated to accommodate this new parameter.
1403-1420: Add prefetch_meta parameter to StringsPersist functionThe
StringsPersistfunction has been updated to include astd::string&& prefetch_metaparameter. This parameter is used to optimize the handling of metadata, potentially reducing the need for additional metadata fetches. This change should be reflected in all calls to this function to ensure consistency.
1441-1458: Add prefetch_meta parameter to StringsTTL functionThe
StringsTTLfunction now includes astd::string&& prefetch_metaparameter. This addition is intended to optimize metadata handling by leveraging prefetched metadata, which can improve performance. Ensure that all relevant function calls are updated to pass this new parameter.
| return Status::InvalidArgument( | ||
| "WRONGTYPE, key: " + keys[idx] + ", expect type: " + | ||
| DataTypeStrings[static_cast<int>(DataType::kSets)] + ", get type: " + | ||
| DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); |
There was a problem hiding this comment.
Consistent issue with error handling across set operations.
The error handling for type mismatches is consistent across various functions but could benefit significantly from a centralized approach or utility function as suggested above. This would ensure that any changes to the error format or logic only need to be made in one place, improving the code's maintainability.
Also applies to: 229-232, 298-301, 321-324, 370-373, 428-431, 454-457, 529-532, 559-562, 614-617, 666-669, 706-709, 716-719, 760-763, 822-825, 868-871, 932-935, 1049-1052, 1123-1126, 1190-1193, 1250-1253, 1292-1295, 1352-1355
| rocksdb::Status Redis::SetsExpire(const Slice& key, int64_t ttl, std::string&& prefetch_meta) { | ||
| std::string meta_value(std::move(prefetch_meta)); | ||
| ScopeRecordLock l(lock_mgr_, key); | ||
|
|
||
| BaseMetaKey base_meta_key(key); | ||
| rocksdb::Status s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | ||
| if (s.ok() && !ExpectedMetaValue(DataType::kSets, meta_value)) { | ||
| if (ExpectedStale(meta_value)) { | ||
| s = Status::NotFound(); | ||
| } else { | ||
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kSets)] + "get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | ||
| rocksdb::Status s; | ||
|
|
||
| // meta_value is empty means no meta value get before, | ||
| // we should get meta first | ||
| if (meta_value.empty()) { | ||
| s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | ||
| if (s.ok() && !ExpectedMetaValue(DataType::kSets, meta_value)) { | ||
| if (ExpectedStale(meta_value)) { | ||
| s = Status::NotFound(); | ||
| } else { | ||
| return Status::InvalidArgument( | ||
| "WRONGTYPE, key: " + key.ToString() + ", expect type: " + | ||
| DataTypeStrings[static_cast<int>(DataType::kSets)] + ", get type: " + | ||
| DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | ||
| } |
There was a problem hiding this comment.
Refactor suggestion for error handling and metadata fetching in expiration and deletion methods.
The pattern of fetching metadata and handling errors is repeated in multiple functions (SetsExpire, SetsDel, SetsExpireat, SetsPersist, SetsTTL). Consider abstracting these common operations into helper functions to reduce code duplication and improve clarity.
+ rocksdb::Status fetchAndValidateMeta(const Slice& key, std::string& meta_value) {
+ BaseMetaKey base_meta_key(key);
+ rocksdb::Status s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value);
+ if (s.ok() && !ExpectedMetaValue(DataType::kSets, meta_value)) {
+ if (ExpectedStale(meta_value)) {
+ return Status::NotFound();
+ } else {
+ return generateTypeError(key, DataType::kSets, GetMetaValueType(meta_value));
+ }
+ }
+ return s;
+ }
- if (meta_value.empty()) {
- s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value);
- if (s.ok() && !ExpectedMetaValue(DataType::kSets, meta_value)) {
- if (ExpectedStale(meta_value)) {
- s = Status::NotFound();
- } else {
- return Status::InvalidArgument(
- "WRONGTYPE, key: " + key.ToString() + ", expect type: " +
- DataTypeStrings[static_cast<int>(DataType::kSets)] + ", get type: " +
- DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]);
- }
- }
- }
+ if (meta_value.empty()) {
+ s = fetchAndValidateMeta(key, meta_value);
+ if (!s.ok()) return s;
+ }Also applies to: 1450-1468, 1487-1505, 1526-1544, 1566-1583
| rocksdb::Status Redis::SCard(const Slice& key, int32_t* ret, std::string&& meta) { | ||
| *ret = 0; | ||
| std::string meta_value; | ||
|
|
||
| BaseMetaKey base_meta_key(key); | ||
| rocksdb::Status s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | ||
| if (s.ok() && !ExpectedMetaValue(DataType::kSets, meta_value)) { | ||
| if (ExpectedStale(meta_value)) { | ||
| s = Status::NotFound(); | ||
| } else { | ||
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kSets)] + "get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | ||
| std::string meta_value(std::move(meta)); | ||
| rocksdb::Status s; | ||
| if (meta_value.empty()) { | ||
| BaseMetaKey base_meta_key(key); | ||
| s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | ||
| if (s.ok() && !ExpectedMetaValue(DataType::kSets, meta_value)) { | ||
| if (ExpectedStale(meta_value)) { | ||
| s = Status::NotFound(); | ||
| } else { | ||
| return Status::InvalidArgument( | ||
| "WRONGTYPE, key: " + key.ToString() + ", expect type: " + | ||
| DataTypeStrings[static_cast<int>(DataType::kSets)] + ", get type: " + | ||
| DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | ||
| } |
There was a problem hiding this comment.
Potential issue with handling of metadata prefetch parameter in SCard.
The function now accepts a std::string&& meta parameter for metadata prefetching. However, there's a logical inconsistency:
- If
meta_valueis empty, the code fetches metadata from the database. This behavior should be explicitly documented or handled more gracefully to avoid confusion. - The error message for type mismatch (
WRONGTYPE) is repeated multiple times across various functions. Consider refactoring to a helper function to reduce redundancy and improve maintainability.
- "WRONGTYPE, key: " + key.ToString() + ", expect type: " +
- DataTypeStrings[static_cast<int>(DataType::kSets)] + ", get type: " +
- DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]);
+ generateTypeError(key, DataType::kSets, GetMetaValueType(meta_value));
// Helper function
std::string generateTypeError(const Slice& key, DataType expected, DataType actual) {
return fmt::format("WRONGTYPE, key: {}, expect type: {}, get type: {}",
key.ToString(),
DataTypeStrings[static_cast<int>(expected)],
DataTypeStrings[static_cast<int>(actual)]);
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rocksdb::Status Redis::SCard(const Slice& key, int32_t* ret, std::string&& meta) { | |
| *ret = 0; | |
| std::string meta_value; | |
| BaseMetaKey base_meta_key(key); | |
| rocksdb::Status s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | |
| if (s.ok() && !ExpectedMetaValue(DataType::kSets, meta_value)) { | |
| if (ExpectedStale(meta_value)) { | |
| s = Status::NotFound(); | |
| } else { | |
| return Status::InvalidArgument("WRONGTYPE, key: " + key.ToString() + ", expect type: " + DataTypeStrings[static_cast<int>(DataType::kSets)] + "get type: " + DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | |
| std::string meta_value(std::move(meta)); | |
| rocksdb::Status s; | |
| if (meta_value.empty()) { | |
| BaseMetaKey base_meta_key(key); | |
| s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | |
| if (s.ok() && !ExpectedMetaValue(DataType::kSets, meta_value)) { | |
| if (ExpectedStale(meta_value)) { | |
| s = Status::NotFound(); | |
| } else { | |
| return Status::InvalidArgument( | |
| "WRONGTYPE, key: " + key.ToString() + ", expect type: " + | |
| DataTypeStrings[static_cast<int>(DataType::kSets)] + ", get type: " + | |
| DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]); | |
| } | |
| rocksdb::Status Redis::SCard(const Slice& key, int32_t* ret, std::string&& meta) { | |
| *ret = 0; | |
| std::string meta_value(std::move(meta)); | |
| rocksdb::Status s; | |
| if (meta_value.empty()) { | |
| BaseMetaKey base_meta_key(key); | |
| s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | |
| if (s.ok() && !ExpectedMetaValue(DataType::kSets, meta_value)) { | |
| if (ExpectedStale(meta_value)) { | |
| s = Status::NotFound(); | |
| } else { | |
| return Status::InvalidArgument(generateTypeError(key, DataType::kSets, GetMetaValueType(meta_value))); | |
| } | |
| } | |
| } | |
| // Rest of the function... | |
| } | |
| // Helper function | |
| std::string generateTypeError(const Slice& key, DataType expected, DataType actual) { | |
| return fmt::format("WRONGTYPE, key: {}, expect type: {}, get type: {}", | |
| key.ToString(), | |
| DataTypeStrings[static_cast<int>(expected)], | |
| DataTypeStrings[static_cast<int>(actual)]); | |
| } |
| @@ -211,17 +217,25 @@ Status Redis::LInsert(const Slice& key, const BeforeOrAfter& before_or_after, co | |||
| return s; | |||
| } | |||
|
|
|||
| Status Redis::LLen(const Slice& key, uint64_t* len) { | |||
| Status Redis::LLen(const Slice& key, uint64_t* len, std::string&& prefetch_meta) { | |||
There was a problem hiding this comment.
Refactor: Standardize error handling for data type mismatches.
The error handling for incorrect data type is repeated across various functions. Consider refactoring this into a helper function to reduce code duplication and improve maintainability.
- return Status::InvalidArgument(
- "WRONGTYPE, key: " + key.ToString() + ", expect type: " +
- DataTypeStrings[static_cast<int>(DataType::kLists)] + ", get type: " +
- DataTypeStrings[static_cast<int>(GetMetaValueType(meta_value))]);
+ return HandleWrongType(DataType::kLists, meta_value, key);Also applies to: 222-222, 224-224, 225-225, 226-226, 227-227, 228-228, 229-229, 230-230, 231-231, 232-232, 233-233, 234-234, 235-235, 236-236, 237-237, 238-238
…penAtomFoundation#2744) Co-authored-by: wangshaoyi <[email protected]>
fix bugs mentioned in #2735
Summary by CodeRabbit
New Features
Refactor
prefetch_metaparameter, optimizing metadata handling.Bug Fixes
exists = trueand returningStatus::OK()in specific conditions.Performance Improvements