feat:add RTC command#3153
Conversation
WalkthroughIsInterceptedByRTC was refactored to use two in-function static command-name sets (string and hash). The function returns true if the command is a member of the appropriate set and the corresponding g_pika_conf cache flag (GetCacheString or GetCacheHash) is enabled; list handling was removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as Client
participant PC as PikaClientConn
participant CS as Command Sets (string/hash)
participant CC as Cache Config
C->>PC: Send command
PC->>CS: Check membership (string or hash set)
alt In string set
PC->>CC: Query GetCacheString
alt Enabled
note right of PC #D6EAF8: Intercept via RTC (string)
PC-->>C: Intercepted (RTC)
else Disabled
note right of PC #F8EBE0: Skip interception
PC-->>C: Normal handling
end
else In hash set
PC->>CC: Query GetCacheHash
alt Enabled
note right of PC #D6EAF8: Intercept via RTC (hash)
PC-->>C: Intercepted (RTC)
else Disabled
note right of PC #F8EBE0: Skip interception
PC-->>C: Normal handling
end
else Not in any set
note right of PC #F5F5F5: Not interceptable
PC-->>C: Normal handling
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 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). (5)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pika_client_conn.cc (1)
6-10: Add missing header for std::unordered_set.This TU uses std::unordered_set but doesn’t include its header. Relying on transitive includes is brittle and may fail CI with -Werror/-Wpedantic.
#include <utility> #include <vector> +#include <unordered_set>
🧹 Nitpick comments (1)
src/pika_client_conn.cc (1)
389-407: Refresh the outdated comment to reflect the widened interception set.The comment still claims only Get/HGet reach the cache path.
- // dont store cmd with too large key(only Get/HGet cmd can reach here) - // the cmd with large key should be non-exist in cache, except for pre-stored + // Don't store cmds with too-large keys. + // Only read-only cmds reach here via RTC (e.g., GET/STRLEN/TTL, HGET/HMGET/HEXISTS/HVALS/HSTRLEN, LLEN). + // Large keys should be non-existent in cache, except for pre-stored entries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/pika_client_conn.cc(1 hunks)
⏰ 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). (3)
- GitHub Check: Analyze (go)
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_centos
🔇 Additional comments (1)
src/pika_client_conn.cc (1)
275-287: Cache fast-path available for all newly intercepted commands. STRLEN, TTL, HGET, HEXISTS, HMGET, HSTRLEN, HVALS and LLEN are all registered withkCmdFlagsReadCache(and fast-path flags) and will be handled byCmd::DoReadCommandInCache(). TTL remains cached—verify that any potential staleness meets your requirements.
| bool PikaClientConn::IsInterceptedByRTC(std::string& opt) { | ||
| // currently we only Intercept: Get, HGet | ||
| if (opt == kCmdNameGet && g_pika_conf->GetCacheString()) { | ||
|
|
||
| static const std::unordered_set<std::string> intercepted_string_cmds = { | ||
| kCmdNameGet, kCmdNameStrlen, kCmdNameTtl | ||
| }; | ||
|
|
||
| static const std::unordered_set<std::string> intercepted_hash_cmds = { | ||
| kCmdNameHGet, kCmdNameHMget, kCmdNameHExists, kCmdNameHVals, kCmdNameHStrlen | ||
| }; | ||
|
|
||
| static const std::unordered_set<std::string> intercepted_list_cmds = { | ||
| kCmdNameLLen | ||
| }; | ||
|
|
||
| if (intercepted_string_cmds.count(opt) && g_pika_conf->GetCacheString()) { | ||
| return true; | ||
| } | ||
| if (opt == kCmdNameHGet && g_pika_conf->GetCacheHash()) { | ||
| if (intercepted_hash_cmds.count(opt) && g_pika_conf->GetCacheHash()) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make opt a const reference and wire up list interception (or remove the dead set).
- Parameter is not modified; use const std::string& to avoid implying mutation.
- intercepted_list_cmds is defined but never used; either gate it with a list cache flag or drop it to avoid warnings.
-bool PikaClientConn::IsInterceptedByRTC(std::string& opt) {
+bool PikaClientConn::IsInterceptedByRTC(const std::string& opt) {
@@
if (intercepted_hash_cmds.count(opt) && g_pika_conf->GetCacheHash()) {
return true;
}
+ // If list cache is supported, enable interception for list reads
+ if (intercepted_list_cmds.count(opt) && g_pika_conf->GetCacheList()) {
+ return true;
+ }
+
return false;
}If GetCacheList() doesn’t exist yet, remove intercepted_list_cmds for now to prevent dead code warnings.
📝 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.
| bool PikaClientConn::IsInterceptedByRTC(std::string& opt) { | |
| // currently we only Intercept: Get, HGet | |
| if (opt == kCmdNameGet && g_pika_conf->GetCacheString()) { | |
| static const std::unordered_set<std::string> intercepted_string_cmds = { | |
| kCmdNameGet, kCmdNameStrlen, kCmdNameTtl | |
| }; | |
| static const std::unordered_set<std::string> intercepted_hash_cmds = { | |
| kCmdNameHGet, kCmdNameHMget, kCmdNameHExists, kCmdNameHVals, kCmdNameHStrlen | |
| }; | |
| static const std::unordered_set<std::string> intercepted_list_cmds = { | |
| kCmdNameLLen | |
| }; | |
| if (intercepted_string_cmds.count(opt) && g_pika_conf->GetCacheString()) { | |
| return true; | |
| } | |
| if (opt == kCmdNameHGet && g_pika_conf->GetCacheHash()) { | |
| if (intercepted_hash_cmds.count(opt) && g_pika_conf->GetCacheHash()) { | |
| return true; | |
| } | |
| return false; | |
| bool PikaClientConn::IsInterceptedByRTC(const std::string& opt) { | |
| static const std::unordered_set<std::string> intercepted_string_cmds = { | |
| kCmdNameGet, kCmdNameStrlen, kCmdNameTtl | |
| }; | |
| static const std::unordered_set<std::string> intercepted_hash_cmds = { | |
| kCmdNameHGet, kCmdNameHMget, kCmdNameHExists, kCmdNameHVals, kCmdNameHStrlen | |
| }; | |
| static const std::unordered_set<std::string> intercepted_list_cmds = { | |
| kCmdNameLLen | |
| }; | |
| if (intercepted_string_cmds.count(opt) && g_pika_conf->GetCacheString()) { | |
| return true; | |
| } | |
| if (intercepted_hash_cmds.count(opt) && g_pika_conf->GetCacheHash()) { | |
| return true; | |
| } | |
| // If list cache is supported, enable interception for list reads | |
| if (intercepted_list_cmds.count(opt) && g_pika_conf->GetCacheList()) { | |
| return true; | |
| } | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/pika_client_conn.cc around lines 275 to 296, change the function
signature to take opt as a const std::string& (since it is not modified) and
eliminate the unused intercepted_list_cmds to avoid dead-code warnings; if a
GetCacheList() config accessor exists, instead wire intercepted_list_cmds into
the logic the same way as strings and hashes (check membership and return true
when GetCacheList() is enabled), otherwise remove the intercepted_list_cmds
definition and any references to it.
# Conflicts: # src/pika_client_conn.cc
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pika_client_conn.cc (1)
275-275: Make opt a const reference.The parameter isn’t modified. Prefer const std::string& to avoid accidental mutation and enable binding to temporaries.
Apply:
-bool PikaClientConn::IsInterceptedByRTC(std::string& opt) { +bool PikaClientConn::IsInterceptedByRTC(const std::string& opt) {Remember to update the declaration in the corresponding header as well.
🧹 Nitpick comments (2)
src/pika_client_conn.cc (2)
289-297: Micro: short‑circuit on disabled caches before hashing.Avoid set lookups when the related cache is off.
Example:
- if (intercepted_string_cmds.count(opt) && g_pika_conf->GetCacheString()) { + if (g_pika_conf->GetCacheString() && intercepted_string_cmds.count(opt)) { return true; } - if (intercepted_hash_cmds.count(opt) && g_pika_conf->GetCacheHash()) { + if (g_pika_conf->GetCacheHash() && intercepted_hash_cmds.count(opt)) { return true; } - if (intercepted_list_cmds.count(opt) && g_pika_conf->GetCacheList()) { + if (g_pika_conf->GetCacheList() && intercepted_list_cmds.count(opt)) { return true; }
277-288: Restrict intercepted_*_cmds to GET and HGET only
Only GET and HGET reach the RTC read path (per the “only read command(Get, HGet) will reach here” comment), so remove Strlen, Ttl, HMget, HExists, HVals, HStrlen and LLen from the intercepted_*_cmds sets to avoid spurious cache_miss_in_rtc flags.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/pika_client_conn.cc(1 hunks)
⏰ 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). (3)
- GitHub Check: build_on_centos
- GitHub Check: build_on_ubuntu
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
src/pika_client_conn.cc (2)
295-297: Nice: list interception is now gated on GetCacheList.This wires previously unused intercepted_list_cmds into config properly.
292-297: Confirmed GetCacheList() exists in PikaConf. No further action required.
| static const std::unordered_set<std::string> intercepted_string_cmds = { | ||
| kCmdNameGet, kCmdNameStrlen, kCmdNameTtl | ||
| }; | ||
|
|
||
| static const std::unordered_set<std::string> intercepted_hash_cmds = { | ||
| kCmdNameHGet, kCmdNameHMget, kCmdNameHExists, kCmdNameHVals, kCmdNameHStrlen | ||
| }; | ||
|
|
||
| static const std::unordered_set<std::string> intercepted_list_cmds = { | ||
| kCmdNameLLen | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add the missing header for std::unordered_set.
Relying on transitive includes is brittle; explicitly include the header.
Apply near the other STL includes:
#include <utility>
#include <vector>
+#include <unordered_set>📝 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.
| static const std::unordered_set<std::string> intercepted_string_cmds = { | |
| kCmdNameGet, kCmdNameStrlen, kCmdNameTtl | |
| }; | |
| static const std::unordered_set<std::string> intercepted_hash_cmds = { | |
| kCmdNameHGet, kCmdNameHMget, kCmdNameHExists, kCmdNameHVals, kCmdNameHStrlen | |
| }; | |
| static const std::unordered_set<std::string> intercepted_list_cmds = { | |
| kCmdNameLLen | |
| }; | |
| #include <utility> | |
| #include <vector> | |
| #include <unordered_set> |
🤖 Prompt for AI Agents
In src/pika_client_conn.cc around lines 277 to 288, the code uses
std::unordered_set but doesn't include its header; add an explicit #include
<unordered_set> near the other STL includes at the top of the file (grouped with
headers like <string>, <vector>, etc.) to avoid relying on transitive includes
and ensure the symbol is defined.
| kCmdNameHGet, kCmdNameHMget, kCmdNameHExists, kCmdNameHVals, kCmdNameHStrlen | ||
| }; | ||
|
|
||
| static const std::unordered_set<std::string> intercepted_list_cmds = { |
There was a problem hiding this comment.
这里为什么是这些命令可以走 RTC 呢，具体的挑选逻辑可以列一下，然后 Zset 和 Set 类型我看好像没有添加上去
There was a problem hiding this comment.
https://geelib.qihoo.net/geelib/knowledge/doc?spaceId=379&docId=198710 ，
带注视的是要加的，zset 和 set没有常用的需要加的命令所以没添加
挑选的逻辑会按照：1只能是读命令并且不改变数值，2.命令访问的所有值都存储在cache中，不可以是一部分cache一部分DB 按照这个逻辑去挑选的
There was a problem hiding this comment.
LLEN 命令能确保 List 的数据都在 Cache 里面吗，感觉计算长度这些命令不应该走 RTC 这种，因为如果内存占用满了会发生淘汰
There was a problem hiding this comment.
LLEN 命令能确保 List 的数据都在 Cache 里面吗，感觉计算长度这些命令不应该走 RTC 这种，因为如果内存占用满了会发生淘汰
list的key的value值要不全部在cache要不全部在db中不会有一部分在cache一部分在db，这个命令走不走都可以吧，反正请求量也不会特别大
Removed unused intercepted_list_cmds set from pika_client_conn.cc.
Removed unused intercepted_list_cmds set from pika_client_conn.cc. --------- Co-authored-by: chejinge <[email protected]>
4 participants
const std::string kCmdNameGet = "get"; //1
const std::string kCmdNameStrlen = "strlen";//1
// Hash
const std::string kCmdNameHGet = "hget"; //1
const std::string kCmdNameHGetall = "hgetall"; //1
const std::string kCmdNameHExists = "hexists"; //1
const std::string kCmdNameHKeys = "hkeys";//1
const std::string kCmdNameHLen = "hlen";//1
const std::string kCmdNameHMget = "hmget";//1
const std::string kCmdNameHStrlen = "hstrlen";//1
const std::string kCmdNameHVals = "hvals";//1
// List
const std::string kCmdNameLLen = "llen";//1
// BitMap
const std::string kCmdNameBitCount = "bitcount";//1
// Zset
const std::string kCmdNameZCount = "zcount";//1
// Set
Summary by CodeRabbit