feat:rtc can intercept more command #3033
Conversation
WalkthroughThis pull request refactors the logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Conn as PikaClientConn
participant CmdMgr as g_pika_cmd_table_manager
participant Config as g_pika_conf
Client->>Conn: Send command (opt)
Conn->>CmdMgr: Retrieve command object (GetCmd(opt))
CmdMgr-->>Conn: Return command object / null
alt Command found
Conn->>Conn: Evaluate IsNeedReadCache()
alt Cache read required
Conn->>Config: Check corresponding flag(s)
Config-->>Conn: Return flag status
Conn-->>Client: Return interception decision based on flag
else Cache read not required
Conn-->>Client: Return false
end
else Command not found
Conn-->>Client: Return false
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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 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 (2)
src/pika_client_conn.cc (2)
261-294: Consider unifying repetitive checks to improve readability.
The new interception logic checks each flag separately for cache configuration and returns early if a match is found. While it works, you might de-duplicate these blocks and handle multi-flag scenarios more cleanly by aggregating all relevant conditions into one or two consolidated checks. This makes the code more maintainable and reduces potential oversights when adding new flags.Possible approach:
if (c_ptr->IsNeedReadCache()) { - if (c_ptr->flag_ & kCmdFlagsOperateKey) { - return true; - } - if (c_ptr->flag_ & kCmdFlagsKv) { - if (g_pika_conf->GetCacheString()) { - return true; - } - } else if (c_ptr->flag_ & kCmdFlagsZset) { - ... - } + const bool interceptable = + (c_ptr->flag_ & kCmdFlagsOperateKey) + || ((c_ptr->flag_ & kCmdFlagsKv) && g_pika_conf->GetCacheString()) + || ((c_ptr->flag_ & kCmdFlagsZset) && g_pika_conf->GetCacheZset()) + || ((c_ptr->flag_ & kCmdFlagsHash) && g_pika_conf->GetCacheHash()) + || ((c_ptr->flag_ & kCmdFlagsList) && g_pika_conf->GetCacheList()) + || ((c_ptr->flag_ & kCmdFlagsBit) && g_pika_conf->GetCacheBit()); + if (interceptable) { + return true; + } }
377-393: Add logging or handling for large-key skips.
It's prudent to skip storing oversized keys in the cache. For operational insight, consider adding a log entry or counter increment whenever a large key is encountered. This can help operators understand when keys are not being cached due to size constraints and avoid potential confusion if expected items fail to appear in the cache.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pika_client_conn.cc(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_macos
- GitHub Check: Analyze (go)
- GitHub Check: build_on_centos
|
这个改动不一定合理，我看了前面的相关pr，有提到rtc最好是拦截单key。同时如果是但是哪怕纯内存也比较慢的操作不能走rtc。 |
This change is not necessarily reasonable. I looked at the previous related PR and mentioned that RTC is best to intercept single keys. At the same time, if it is, even if it is pure memory, it is relatively slow, it cannot use rtc. |
将rtc扩展为支持ReadCache的所有命令。
Summary by CodeRabbit