ZPOPMAX Command Migration #1655
Conversation
WalkthroughThe pull request introduces a new command implementation for Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CR as Command Registry
participant EX as executeZPOPMAX
participant EV as evalZPOPMAX
participant PS as parseZPOPMAXArgs
participant SS as SortedSet
U->>CR: Submit "ZPOPMAX" command with args
CR->>EX: Invoke executeZPOPMAX
EX->>EV: Delegate processing
EV->>PS: Parse and validate arguments
PS-->>EV: Return key and count (or error)
EV->>SS: Call PopMax(key, count)
SS-->>EV: Return highest-scoring members
EV-->>EX: Return command result
EX-->>CR: Command execution complete
CR-->>U: Output result
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: 0
🧹 Nitpick comments (5)
internal/cmd/cmds.go (1)
196-204: Clean implementation of slice response conversion.The
cmdResSlicefunction elegantly handles the conversion from Go strings to protocol buffer values. Consider adding a capacity hint to thevaluesslice initialization to optimize memory allocation when the result size is known:-var values []*structpb.Value +var values = make([]*structpb.Value, 0, len(res))internal/cmd/cmd_zpopmax.go (1)
64-77: Input parsing is robust but consider naming return parameters.
- Checks argument count to avoid misuse.
- Converts the
countargument and validates it (must be > 0).- Returns defaults if
countisn’t provided.Additionally, static analysis suggests naming the return parameters (e.g.,
func parseZPOPMAXArgs(c *Cmd) (key string, count int, err error)) for clarity. This is a minor improvement for readability.🧰 Tools
🪛 golangci-lint (1.64.8)
64-64: unnamedResult: consider giving a name to these results
(gocritic)
internal/cmd/cmd_zadd.go (3)
15-29: Minor documentation nitpick: Remove repeated "CH" in HelpLong description.
The "CH" option is listed twice in the help text, which may confuse developers.-Options: NX, XX, CH, INCR, GT, LT, CH +Options: NX, XX, CH, INCR, GT, LT
42-62: Input validation is well-handled, but consider better error feedback.
Currently, user errors such as non-numeric scores or malformed arguments yield generic errors. You may want to clarify which argument caused the error to provide more user-friendly feedback.
64-83: Shard retrieval logic is consistent; consider TTL checks if applicable.
The code loads or creates the sorted set, but if your application supports key expirations or time-based evictions, consider verifying or updating TTL here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
internal/cmd/cmd_del.go(2 hunks)internal/cmd/cmd_exists.go(2 hunks)internal/cmd/cmd_expiretime.go(1 hunks)internal/cmd/cmd_hset.go(1 hunks)internal/cmd/cmd_incrby.go(2 hunks)internal/cmd/cmd_ttl.go(1 hunks)internal/cmd/cmd_zadd.go(1 hunks)internal/cmd/cmd_zpopmax.go(1 hunks)internal/cmd/cmds.go(2 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
internal/cmd/cmd_ttl.go (1)
internal/store/expire.go (1)
GetExpiry(29-32)
internal/cmd/cmd_zadd.go (1)
internal/cmd/cmds.go (4)
CommandMeta(72-80)CommandRegistry(94-96)Cmd(24-30)CmdRes(67-70)
internal/cmd/cmd_zpopmax.go (1)
internal/cmd/cmds.go (4)
CommandMeta(72-80)CommandRegistry(94-96)Cmd(24-30)CmdRes(67-70)
🪛 golangci-lint (1.64.8)
internal/cmd/cmd_zpopmax.go
64-64: unnamedResult: consider giving a name to these results
(gocritic)
🔇 Additional comments (26)
internal/cmd/cmds.go (3)
179-184: Excellent abstraction for integer responses.This utility function nicely encapsulates the creation of integer responses, improving code readability and maintainability across command implementations.
186-190: Well-structured floating-point response helper.The
cmdResFloatfunction provides a consistent way to generate floating-point responses, which will be particularly useful for the sorted set operations mentioned in the PR objectives.
192-194: Good addition of empty slice response.This pre-initialized empty slice response object will help optimize performance in cases where commands need to return empty collections.
internal/cmd/cmd_incrby.go (2)
58-58: Good usage of the new utility function.Replacing direct wire.Response construction with
cmdResIntimproves readability and maintainability.
73-73: Consistent implementation with first usage.The second usage of
cmdResIntmaintains consistency with the first usage in the same file.internal/cmd/cmd_del.go (2)
56-56: Appropriate type conversion in cmdResInt call.Good job explicitly converting the count to int64 before passing it to cmdResInt, ensuring type safety.
73-73: Consistent implementation with other commands.This change aligns well with the overall refactoring goal of removing direct wire package usage.
internal/cmd/cmd_exists.go (2)
51-51: Clean implementation using utility function.The use of
cmdResInthere improves code clarity while maintaining the same functionality.
67-67: Good refactoring for response generation.This change is consistent with the pattern established in other command implementations and improves maintainability.
internal/cmd/cmd_hset.go (1)
103-103: Use ofcmdResIntis a clean simplification.Replacing the direct construction of a wire response with
cmdResInt(newFields)uniformly aligns with other commands and reduces code clutter. This refactor is consistent, straightforward, and maintains the previous behavior.internal/cmd/cmd_expiretime.go (1)
59-59: Consistent transition tocmdResInt.Switching to
cmdResInt(int64(expiry / 1000))removes the directwireusage and preserves the integer TTL logic (in seconds). This implementation looks correct and aligns with the rest of the refactor.internal/cmd/cmd_ttl.go (3)
52-52: Return value updated to-2for nonexistent keys.Using
cmdResInt(-2)is a clear and consistent choice for indicating a missing key. No further concerns here.
58-58: Return value updated to-1for keys without expiration.Returning
cmdResInt(-1)cleanly conveys that the key has no associated TTL. Implementation is straightforward and aligned with the command’s specification.
63-63: Consistent integer division for TTL.Using
cmdResInt(int64(durationMs / 1000))ensures the response is in whole seconds. This approach is consistent with typical TTL behavior. No issues noted.internal/cmd/cmd_zpopmax.go (6)
1-2: New package declaration for ZPOPMAX command.The package and blank line are fine. No issues here.
3-10: Dependency imports are appropriate.Imports (e.g.,
sortedset,shardmanager,store) are correct for implementing ZPOPMAX. No redundant imports noted.
12-38: Command metadata is well-defined.The
cZPOPMAXstruct clearly documents the command name, syntax, help text, and examples, making it straightforward for users. The usage examples demonstrate different scenarios, which is beneficial.
40-42: Initialization logic is concise.Registering
cZPOPMAXin the command registry is sufficient. This preserves a consistent pattern with other commands.
44-62: Core logic for removing and returning highest-scoring members.
- Properly checks for a missing key (returns empty).
- Converts the retrieved object to a valid sorted set.
- Calls
PopMax(count)and returns viacmdResSlice.
Overall, the flow is clear and handles error conditions correctly.
79-85: Final execution function is straightforward.Ensures at least one argument is passed and retrieves the appropriate shard before delegating to
evalZPOPMAX. No issues found.internal/cmd/cmd_zadd.go (6)
1-14: No concerns with the package and imports.
Everything here appears standard and correct.
38-41: Command registration looks good.
The command is correctly added to the global registry ininit().
85-115: Flag parsing logic is concise; watch out for flag collisions.
While the parsing logic is clean, you might consider adding tests to confirm that surprising combinations of flags (e.g., using NX with GT) are caught properly.Do you need a script or additional tests to ensure coverage for these edge cases?
134-160: Check return of (nil, nil) for performance and clarity.
When GT/LT conditions fail after anINCR, the code returns(nil, nil)(line 160). This indicates no response is sent to the client. Confirm this is the desired behavior; otherwise, returning a standard response (like an empty array or 0) may be more consistent.
195-202: Condition checks inshouldSkipMemberlook correct.
The logic for NX, XX, GT, LT skipping is correctly combined. Well done!
204-207: Storing updated sets is properly encapsulated.
The update strategy is aligned with the rest of the architecture. No issues found.
|
@lucifercr07 @arpitbbhayani @JyotinderSingh @ayushsatyam146
the key "set" isn’t present in the store, so our code correctly returns an empty slice using:
Since cmdResEmptySlice (or the equivalent message) is empty, proto.Marshal(r) produces an empty byte array (length 0). Consequently, the call to h.conn.Write(b) effectively sends no data over the connection. Possible Solutions: 1. Force a Non-Nil Empty Payload: 2. Send a Sentinel Value: 3. Explicit Write Check: How to replicate:
Let me know if any other clarifications/discussions are required on this |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/cmd/cmd_zpopmax.go (2)
64-64: Consider naming function return valuesThe static analysis tool suggests giving names to function results to improve code readability and documentation.
- func parseZPOPMAXArgs(c *Cmd) (string, int, error) { + func parseZPOPMAXArgs(c *Cmd) (key string, count int, err error) {🧰 Tools
🪛 golangci-lint (1.64.8)
64-64: unnamedResult: consider giving a name to these results
(gocritic)
79-85: Redundant argument validationThe argument validation in
executeZPOPMAXis redundant with the validation inparseZPOPMAXArgs. Consider relying solely on the validation inparseZPOPMAXArgs.func executeZPOPMAX(c *Cmd, sm *shardmanager.ShardManager) (*CmdRes, error) { - if len(c.C.Args) < 1 { - return cmdResNil, errors.ErrWrongArgumentCount("ZPOPMAX") - } shard := sm.GetShardForKey(c.C.Args[0]) return evalZPOPMAX(c, shard.Thread.Store()) }However, this assumes that
GetShardForKeyproperly handles the case whenc.C.Argsis empty. If not, keep the validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/cmd/cmd_zpopmax.go(1 hunks)internal/cmd/cmds.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cmd/cmds.go
🧰 Additional context used
🧬 Code Definitions (1)
internal/cmd/cmd_zpopmax.go (1)
internal/cmd/cmds.go (4)
CommandMeta(73-81)CommandRegistry(95-97)Cmd(24-30)CmdRes(68-71)
🪛 golangci-lint (1.64.8)
internal/cmd/cmd_zpopmax.go
64-64: unnamedResult: consider giving a name to these results
(gocritic)
🔇 Additional comments (3)
internal/cmd/cmd_zpopmax.go (3)
12-38: Good ZPOPMAX command implementation with clear documentationThe command metadata is well-documented with appropriate syntax, short and long help descriptions, and helpful examples that demonstrate command usage.
40-42: Command registration looks goodCorrectly registers the ZPOPMAX command at initialization time.
44-62: Command evaluation logic handles the empty key case correctlyThe implementation properly returns
cmdResEmptySlicewhen the key doesn't exist, which aligns with the PR objective to handle empty sets correctly.
| func parseZPOPMAXArgs(c *Cmd) (string, int, error) { | ||
| if len(c.C.Args) == 0 || len(c.C.Args) > 2 { | ||
| return "", 0, errors.ErrWrongArgumentCount("ZPOPMAX") | ||
| } | ||
| key := c.C.Args[0] | ||
| count := 1 | ||
| if len(c.C.Args) > 1 { | ||
| count, err := strconv.Atoi(c.C.Args[1]) | ||
| if err != nil || count <= 0 { | ||
| return "", 0, errors.ErrIntegerOutOfRange | ||
| } | ||
| } | ||
| return key, count, nil | ||
| } |
There was a problem hiding this comment.
Critical bug: Shadowed count variable
There's a variable shadowing issue in the parseZPOPMAXArgs function. The nested count variable shadows the outer count variable, which means the function will always return 1 regardless of what value the user provides.
Fix the shadowing issue by removing the redeclaration:
count := 1
if len(c.C.Args) > 1 {
- count, err := strconv.Atoi(c.C.Args[1])
+ var err error
+ count, err = strconv.Atoi(c.C.Args[1])
if err != nil || count <= 0 {
return "", 0, errors.ErrIntegerOutOfRange
}
}📝 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.
| func parseZPOPMAXArgs(c *Cmd) (string, int, error) { | |
| if len(c.C.Args) == 0 || len(c.C.Args) > 2 { | |
| return "", 0, errors.ErrWrongArgumentCount("ZPOPMAX") | |
| } | |
| key := c.C.Args[0] | |
| count := 1 | |
| if len(c.C.Args) > 1 { | |
| count, err := strconv.Atoi(c.C.Args[1]) | |
| if err != nil || count <= 0 { | |
| return "", 0, errors.ErrIntegerOutOfRange | |
| } | |
| } | |
| return key, count, nil | |
| } | |
| func parseZPOPMAXArgs(c *Cmd) (string, int, error) { | |
| if len(c.C.Args) == 0 || len(c.C.Args) > 2 { | |
| return "", 0, errors.ErrWrongArgumentCount("ZPOPMAX") | |
| } | |
| key := c.C.Args[0] | |
| count := 1 | |
| if len(c.C.Args) > 1 { | |
| var err error | |
| count, err = strconv.Atoi(c.C.Args[1]) | |
| if err != nil || count <= 0 { | |
| return "", 0, errors.ErrIntegerOutOfRange | |
| } | |
| } | |
| return key, count, nil | |
| } |
🧰 Tools
🪛 golangci-lint (1.64.8)
64-64: unnamedResult: consider giving a name to these results
(gocritic)
|
We have already merged ZPOPMAX with Tests. |
3 participants
Summary by CodeRabbit