Fix typo in store_valkey.go: stingsSliceResults to stringSliceResults#357
Fix typo in store_valkey.go: stingsSliceResults to stringSliceResults#357zlong928 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @zlong928! It looks like this is your first PR to volcano-sh/agentcube 🎉 |
There was a problem hiding this comment.
Pull request overview
Fixes a variable name typo in the Valkey store implementation (pkg/store/store_valkey.go) to improve code clarity and avoid confusion when working with MGET string slice results.
Changes:
- Rename
stingSliceResultstostringSliceResultsinloadSandboxesBySessionIDs.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request corrects a typo by renaming the variable "stingSliceResults" to "stringSliceResults" within the loadSandboxesBySessionIDs function in pkg/store/store_valkey.go. A review comment suggests improving the validation logic for the Valkey MGet operation by checking for an exact match between the number of results and the number of keys requested, rather than just checking if the result count is greater than the key count.
| if len(stringSliceResults) > len(sessionIDKeys) { | ||
| return nil, fmt.Errorf("unexpected MGet result size: %d, param size: %d", len(stringSliceResults), len(sessionIDKeys)) |
There was a problem hiding this comment.
The condition len(stringSliceResults) > len(sessionIDKeys) is insufficient. In Valkey/Redis, the MGET command always returns exactly one result for each key provided, including nil for non-existent keys. Therefore, the length of the result slice should be exactly equal to the number of keys requested. Any discrepancy (either more or fewer results) indicates an unexpected state or protocol error. Using != ensures that the mapping between results and session IDs remains consistent.
| if len(stringSliceResults) > len(sessionIDKeys) { | |
| return nil, fmt.Errorf("unexpected MGet result size: %d, param size: %d", len(stringSliceResults), len(sessionIDKeys)) | |
| if len(stringSliceResults) != len(sessionIDKeys) { | |
| return nil, fmt.Errorf("unexpected MGet result size: %d, param size: %d", len(stringSliceResults), len(sessionIDKeys)) |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #357 +/- ##
==========================================
+ Coverage 47.57% 49.60% +2.03%
==========================================
Files 30 30
Lines 2819 2885 +66
==========================================
+ Hits 1341 1431 +90
+ Misses 1338 1301 -37
- Partials 140 153 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@zlong928 Pls add your DCO when commit |
…alkey.go Signed-off-by: zlong928 <xihuanwenbaoabo@gmail.com>
zlong928
force-pushed
the
fix-typos-store-valkey
branch
from
527c5e0 to
b82625c
Compare
|
/lgtm |
…alkey.go
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Fix typo
stingsSliceResultstostringSliceResultsin store_valkey.go.Which issue(s) this PR fixes:
Fixes #345
Special notes for your reviewer:
Does this PR introduce a user-facing change?: