feat: add blpop cmd #211
feat: add blpop cmd #211
Conversation
WalkthroughThis pull request updates the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant BL as BLPopCmd
participant L as List Storage
C->>BL: Send BLPOP command with keys and timeout
BL->>BL: DoInitial: Extract keys, parse timeout, validate values
BL->>BL: DoCmd: Acquire multi-key record lock
BL->>L: Attempt to pop element for each key
alt Element successfully popped
L-->>BL: Return popped element
BL->>C: Send response with key & element
else No element found or error occurs
alt Error encountered
BL->>C: Send error response
else No element popped
BL->>C: Block client until push operation occurs
end
end
sequenceDiagram
participant Caller as Application
participant S as Storage
participant DB as Database Instance
participant R as Redis Module
Caller->>S: Call LPopWithoutLock(key, count, elements)
S->>S: Clear elements vector
S->>DB: GetDBInstance(key)
DB->>S: Return instance
S->>DB: Invoke LPopWithoutLock(key, count, elements)
DB-->>S: Return status and updated elements
S->>Caller: Return final status and elements
Note over R,DB: Similar non-locking flow implemented within Redis class methods
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 2
🧹 Nitpick comments (2)
src/storage/src/redis_lists.cc (1)
271-315: Consider consolidating the duplicated pop logic.
This newLPopWithoutLockmethod closely mirrors the existingLPopmethod, except for skipping the lock acquisition. To keep the code DRY, consider extracting the shared logic into a helper that both methods can call, reducing maintenance overhead and potential divergence.src/cmd_list.cc (1)
179-198: Check for non-empty key list.
IfBLPopis called without passing any valid keys, the code may behave unexpectedly or never block. Consider validating that at least one key is provided, returning an error otherwise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/cmd_list.cc(1 hunks)src/storage/include/storage/storage.h(1 hunks)src/storage/src/redis.h(1 hunks)src/storage/src/redis_lists.cc(1 hunks)src/storage/src/storage.cc(1 hunks)
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/cmd_list.cc
[performance] 203-203: Variable 'func_' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
🔇 Additional comments (2)
src/storage/src/redis.h (1)
246-246: Method signature added for LPopWithoutLockThe addition of
LPopWithoutLockmethod declaration provides a non-locking variant of the existingLPopoperation. This is a good addition for scenarios where locks need to be managed externally or for implementing blocking operations like the BLPOP command mentioned in the PR.src/cmd_list.cc (1)
200-220: The redesigned blocking pop logic looks correct.
Keys are iterated in sequence, and the client is properly blocked when no elements are available. This aligns well with typicalBLPopsemantics.🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 203-203: Variable 'func_' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/cmd_list.cc (3)
180-199: Implementation looks good, handles key extraction and timeout parsing properly.The implementation correctly extracts keys from arguments, handles timeout validation, and calculates expiration time when needed. The logic follows the same pattern as BRPopCmd, ensuring consistency.
One small enhancement to consider:
Add validation for an empty key list. If client->argv_ has only 2 elements (command name and timeout), the extracted key list will be empty. Consider adding a check like:
std::vector<std::string> keys(client->argv_.begin() + 1, client->argv_.end() - 1); + if (keys.empty()) { + client->SetRes(CmdRes::kErrOther, "wrong number of arguments for 'blpop' command"); + return false; + } client->SetKey(keys);
201-221: Implementation correctly handles list popping and blocking behavior.The implementation properly initializes data structures, acquires locks for all keys, attempts to pop elements from each list, and blocks the client when necessary. The code correctly uses
LPopWithoutLockto match the "BLPOP" functionality (left pop).Consider adding a check to ensure the elements vector isn't empty after a successful call to LPopWithoutLock before accessing elements[0]:
if (s.ok()) { + if (elements.empty()) { + client->SetRes(CmdRes::kErrOther, "unexpected empty response from storage"); + return; + } client->AppendArrayLen(2); client->AppendString(list_key); client->AppendString(elements[0]); return; }This would prevent a potential out-of-bounds access if the storage layer returns an empty vector but still reports success.
🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 203-203: Variable 'func_' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
206-208: Consider clearing the elements vector before each iteration.To prevent potential accumulation of elements from previous iterations, consider clearing the elements vector before each LPopWithoutLock call.
for (auto& list_key : list_keys) { + elements.clear(); storage::Status s = STORE_INST.GetBackend(client->GetCurrentDB())->GetStorage()->LPopWithoutLock(list_key, 1, &elements);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cmd_list.cc(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/cmd_list.cc (6)
src/base_cmd.cc (4)
DoInitial(187-194)DoInitial(187-187)BlockThisClientToWaitLRPush(109-123)BlockThisClientToWaitLRPush(109-110)src/cmd_list.h (8)
client(19-19)client(22-22)client(30-30)client(33-33)client(41-41)client(44-44)client(52-52)client(55-55)src/storage/src/redis.h (3)
keys(219-219)keys(222-222)keys(232-232)src/std/scope_record_lock.h (4)
keys(50-50)keys(51-51)MultiScopeRecordLock(37-37)MultiScopeRecordLock(38-38)src/std/std_string.h (4)
String2int(62-69)String2int(62-62)String2int(87-89)String2int(87-87)src/storage/src/type_iterator.h (1)
s(454-463)
🪛 Cppcheck (2.10-2)
src/cmd_list.cc
[performance] 203-203: Variable 'func_' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
🔇 Additional comments (1)
src/cmd_list.cc (1)
180-221: Implementation successfully mirrors BRPopCmd with appropriate adjustments.The BLPopCmd implementation follows the same pattern as BRPopCmd but correctly uses
LPopWithoutLockinstead ofRPopWithoutLockand the appropriate blocking type. This approach ensures consistency in the codebase and makes the code more maintainable.🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 203-203: Variable 'func_' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
|
还在测试中 |
Still under testing |
|
把 python 测试用例改成 go 测试用例，可以借助 https://www.wenxiaobai.com/chat/tourist |
Change python test cases to go test cases |
3 participants
复用 #48 BRpop的方式实现 BLpop
Summary by CodeRabbit