fix:Type Command Expired Key Consistency#3132
fix:Type Command Expired Key Consistency#3132Mixficsol merged 2 commits intoOpenAtomFoundation:unstablefrom
Conversation
WalkthroughThe Changes
Estimated code review effort1 (<30 minutes) Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 0
🧹 Nitpick comments (1)
src/storage/src/redis_strings.cc (1)
1674-1679: Consider improving error handling and code consistency.While the core logic is correct, there are a few improvements to consider:
- Error handling: The method returns
Status::OK()even when database read operations fail with errors other thanNotFound. Consider handling database errors explicitly.- Code comments: The Chinese comments should be translated to English for consistency with the rest of the codebase.
- Return status consistency: Unlike other Redis operations that return
Status::NotFound()for missing keys, this method always returnsStatus::OK(). While this might be intentional for the TYPE command, ensure this behavior is documented and consistent with the expected API contract.- // 检查键是否已过期 - if (ExpectedStale(meta_value)) { - type = DataType::kNones; // 如果键已过期，返回"none"类型 - } else { - type = static_cast<enum DataType>(static_cast<uint8_t>(meta_value[0])); - } + // Check if the key has expired + if (ExpectedStale(meta_value)) { + type = DataType::kNones; // Return "none" type if key is expired + } else { + type = static_cast<enum DataType>(static_cast<uint8_t>(meta_value[0])); + }- type = DataType::kNones; // 如果键不存在，返回"none"类型 + type = DataType::kNones; // Return "none" type if key does not exist
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/storage/src/redis_strings.cc(1 hunks)src/storage/src/storage.cc(1 hunks)
🔇 Additional comments (3)
src/storage/src/storage.cc (1)
1517-1526: Clean wrapper implementation that provides string-based type interface.The new
Typemethod effectively wraps the existingGetTypefunctionality and provides a consistent string-based interface. The implementation correctly:
- Clears the output vector before use
- Delegates to the existing
GetTypemethod (which should now handle expiration checks per the PR objectives)- Converts the
DataTypeenum to its string representation- Returns the appropriate status
This aligns well with the PR's goal to fix TYPE command expired key consistency by leveraging the updated
GetTypemethod.src/storage/src/redis_strings.cc (2)
1669-1684: LGTM! The implementation correctly handles expired keys for the TYPE command.The logic properly checks for key expiration using
ExpectedStale(meta_value)and returnsDataType::kNonesfor both expired and non-existent keys, which aligns with the PR objective to fix the TYPE command consistency issue.
1672-1684: No changes needed: TYPE returns “none” for missing/expired keysThe storage layer already sets
DataType::kNonesand always returnsStatus::OK(), andStorage::Typeonly pushes the stringified type on OK. That means:
- GetType(key, type) → OK +
type == kNonesfor non-existent/expired keys- Type(key, types) →
types = { "none" }This matches Redis’s TYPE semantics.
| rocksdb::Status s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | ||
| if (s.ok()) { | ||
| type = static_cast<enum DataType>(static_cast<uint8_t>(meta_value[0])); | ||
| // 检查键是否已过期 |
| return Status::OK(); | ||
| } | ||
|
|
||
| Status Storage::Type(const std::string& key, std::vector<std::string>& types) { |
There was a problem hiding this comment.
这里为什么要新增加一个Type，不可以直接用之前的GetType接口吗
There was a problem hiding this comment.
因为 rocksdb::Status Type() 这个接口在头文件里一直存在，并且已经被大量外部模块（如 pika_migrate、pika_cache、rsync_client 等）调用，它返回的是 Redis 协议要求的字符串类型（如 “string” / “hash”），而旧的 GetType()函数 只返回内部枚举 DataType。
|
done @Mixficsol |
* Fix Type Command Expired Key Consistency * Change to English comments
2 participants
#3130
在PikiwiDB v4中，当哈希类型的键过期时，在使用类似HLEN等命令时会返回0或空结果，表示键不存在。但当使用TYPE命令时，仍然会返回"hash"而不是"none"，这与v355版本的行为不一致。
问题原因：
在Redis::GetType方法中，即使键已过期，也没有检查过期状态，而是直接返回键的类型。元数据中记录了键的类型，即使键过期，这个元数据仍然存在，只是被标记为过期(stale)。
解决方案：
1.修改了Redis::GetType方法，添加了对键是否过期的检查。
2.实现了Storage::Type方法以确保TYPE命令能正确处理过期键。
修复后的效果：
1.现在当哈希键过期时，TYPE命令会返回"none"而不是"hash"
2.这与v355版本的行为一致，避免了在应用程序中使用TYPE命令判断键类型时的混淆
3.解决方案保持了PikiwiDB内部代码的一致性，确保所有命令对过期键的处理逻辑相同
Summary by CodeRabbit