Fix adjacent slot range behavior in ASM operations#24
Conversation
WalkthroughRefactors slot range validation into a normalization-and-validate function that sorts and merges adjacent ranges. Updates import task creation to transfer slot range ownership with centralized error cleanup. Adds utility function to count keys within slot ranges and integrates it into logging and active trim operations. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Parser
participant Normalization
participant ImportTask
participant ErrorHandler
Client->>Parser: parseSlotRangesOrReply(slots)
Parser->>Normalization: slotRangeArrayNormalizeAndValidate(slots)
alt Validation Success
Normalization->>Normalization: Sort ranges by start
Normalization->>Normalization: Merge adjacent ranges
Normalization-->>Parser: Return success
Parser->>ImportTask: Create task with normalized slots
ImportTask-->>Client: Task created, ownership transferred
else Validation Failure
Normalization-->>ErrorHandler: Return error message
ErrorHandler->>ErrorHandler: Free slot ranges
ErrorHandler-->>Client: Return error response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cluster.c (1)
1876-1889: Bug:num_rangescomparison occurs before sort/merge, causing false negatives.The equality check
if (slots1->num_ranges != slots2->num_ranges) return 0;happens before the sort and merge operations. After merging, ranges with different initial counts could become equal.For example:
slots1 = [{0,100}, {101,200}](2 ranges) → merges to[{0,200}](1 range)slots2 = [{0,200}](1 range)These represent identical slots but the function returns
0(not equal) because2 != 1.🐛 Proposed fix
int slotRangeArrayIsEqual(slotRangeArray *slots1, slotRangeArray *slots2) { - if (slots1->num_ranges != slots2->num_ranges) return 0; - slotRangeArraySortAndMerge(slots1); slotRangeArraySortAndMerge(slots2); + if (slots1->num_ranges != slots2->num_ranges) return 0; + for (int i = 0; i < slots1->num_ranges; i++) {
🤖 Fix all issues with AI agents
In `@src/cluster_asm.c`:
- Around line 1011-1023: The local variable total_ranges in asmCountKeysInSlots
is declared but never used; remove the unused declaration to fix the
-Werror=unused-variable CI failure. Edit the function asmCountKeysInSlots and
delete the line declaring int total_ranges = slots->num_ranges; (or
alternatively use total_ranges in the outer loop condition if you prefer),
leaving the existing loops and key_count logic unchanged.
In `@src/cluster.c`:
- Around line 2014-2019: The validation error string returned in err from
slotRangeArrayNormalizeAndValidate is being freed and discarded instead of being
sent back to the client; update parseSlotRangesOrReply so when
slotRangeArrayNormalizeAndValidate(...) != C_OK it calls the client reply API
(e.g., addReplyError(c, err) or addReplyErrorFormat using the sds) before
freeing err and slotRangeArrayFree(slots), then return NULL; this ensures the
caller/client receives the validation message.
🧹 Nitpick comments (1)
src/cluster.c (1)
1879-1880: Consider documenting or avoiding mutation of input arrays.
slotRangeArrayIsEqualnow mutates both input arrays viaslotRangeArraySortAndMerge. This side effect is unexpected for a comparison function and could cause issues if callers reuse the arrays after comparison.Consider either:
- Adding a doc comment warning about the mutation, or
- Working on copies if preserving original arrays is important
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/cluster.csrc/cluster.hsrc/cluster_asm.ctests/unit/cluster/atomic-slot-migration.tcl
🧰 Additional context used
🧬 Code graph analysis (3)
src/cluster.h (1)
src/cluster.c (2)
slotRangeArraySortAndMerge(1860-1873)slotRangeArrayNormalizeAndValidate(1736-1771)
src/cluster_asm.c (4)
src/cluster_legacy.c (1)
getMyClusterNode(5832-5834)src/sds.c (3)
sdsnew(203-206)sdscatprintf(635-642)sdsempty(198-200)src/cluster.c (3)
slotRangeArrayFree(1944-1946)slotRangeArrayDup(1781-1785)slotRangeArrayNormalizeAndValidate(1736-1771)src/kvstore.c (1)
kvstoreDictSize(706-712)
src/cluster.c (2)
src/sds.c (3)
sdscatprintf(635-642)sdsempty(198-200)sdsfree(214-217)src/networking.c (1)
addReplyErrorArity(704-707)
🪛 GitHub Actions: CI
src/cluster_asm.c
[error] 1016-1016: unused variable 'total_ranges' [-Werror=unused-variable]. Variable is assigned but never used in asmCountKeysInSlots.
🪛 GitHub Actions: External Server Tests
src/cluster_asm.c
[error] 1016-1016: unused variable 'total_ranges' [-Werror=unused-variable]
⏰ 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). (4)
- GitHub Check: build-old-chain-jemalloc
- GitHub Check: code-coverage
- GitHub Check: Analyze (cpp)
- GitHub Check: code-coverage
🔇 Additional comments (10)
src/cluster.h (1)
187-197: LGTM!The new function declarations are correctly placed alongside other
slotRangeArrayfunctions and follow the existing naming conventions. The signatures align with the implementations incluster.c.src/cluster.c (4)
1734-1771: LGTM!The normalization and validation logic is well-structured: sort/merge first, then validate bounds and detect duplicates using the bitmap. Error messages are clear and informative.
1855-1873: LGTM!The sort-and-merge implementation is efficient and correct. The in-place algorithm with
idxtracking is a good approach, and the explicit note about overlapping ranges not being merged is helpful.
1796-1796: LGTM!Good defensive check for NULL or empty input, returning an empty string gracefully.
1986-1997: LGTM!The stricter arity validation ensures at least one complete slot range pair is provided and handles edge cases properly.
tests/unit/cluster/atomic-slot-migration.tcl (1)
514-548: LGTM!Comprehensive test coverage for the new sorting and merging functionality. The three test cases effectively cover:
- Simple adjacent merge
- Unsorted input with partial adjacency
- Multiple single-slot ranges merging into a contiguous range
Good verification that both source and destination nodes report the correctly merged slot ranges.
src/cluster_asm.c (4)
798-847: Ownership transfer + centralized cleanup looks solid.
The slots ownership change and cleanup path read cleanly and align with the updated flow.
1039-1055: Helpful key-count logging.
Including key counts at import/migrate boundaries improves observability.
2869-2898: Import-start normalization/validation flow looks good.
Dup + normalize + validate before task creation is clean and consistent.
3306-3307: Consistent key counting via helper.
ReusingasmCountKeysInSlotskeeps trim metrics aligned with logging.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| /* Return the number of keys in the specified slot ranges. */ | ||
| unsigned long long asmCountKeysInSlots(slotRangeArray *slots) { | ||
| if (!slots) return 0; | ||
|
|
||
| unsigned long long key_count = 0; | ||
| int total_ranges = slots->num_ranges; | ||
| for (int i = 0; i < slots->num_ranges; i++) { | ||
| for (int j = slots->ranges[i].start; j <= slots->ranges[i].end; j++) { | ||
| key_count += kvstoreDictSize(server.db[0].keys, j); | ||
| } | ||
| } | ||
| return key_count; | ||
| } |
There was a problem hiding this comment.
Fix CI failure: remove unused total_ranges.
Line 1016 triggers -Werror=unused-variable in CI.
🛠️ Proposed fix
- int total_ranges = slots->num_ranges;📝 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.
| /* Return the number of keys in the specified slot ranges. */ | |
| unsigned long long asmCountKeysInSlots(slotRangeArray *slots) { | |
| if (!slots) return 0; | |
| unsigned long long key_count = 0; | |
| int total_ranges = slots->num_ranges; | |
| for (int i = 0; i < slots->num_ranges; i++) { | |
| for (int j = slots->ranges[i].start; j <= slots->ranges[i].end; j++) { | |
| key_count += kvstoreDictSize(server.db[0].keys, j); | |
| } | |
| } | |
| return key_count; | |
| } | |
| /* Return the number of keys in the specified slot ranges. */ | |
| unsigned long long asmCountKeysInSlots(slotRangeArray *slots) { | |
| if (!slots) return 0; | |
| unsigned long long key_count = 0; | |
| for (int i = 0; i < slots->num_ranges; i++) { | |
| for (int j = slots->ranges[i].start; j <= slots->ranges[i].end; j++) { | |
| key_count += kvstoreDictSize(server.db[0].keys, j); | |
| } | |
| } | |
| return key_count; | |
| } |
🧰 Tools
🪛 GitHub Actions: CI
[error] 1016-1016: unused variable 'total_ranges' [-Werror=unused-variable]. Variable is assigned but never used in asmCountKeysInSlots.
🪛 GitHub Actions: External Server Tests
[error] 1016-1016: unused variable 'total_ranges' [-Werror=unused-variable]
🤖 Prompt for AI Agents
In `@src/cluster_asm.c` around lines 1011 - 1023, The local variable total_ranges
in asmCountKeysInSlots is declared but never used; remove the unused declaration
to fix the -Werror=unused-variable CI failure. Edit the function
asmCountKeysInSlots and delete the line declaring int total_ranges =
slots->num_ranges; (or alternatively use total_ranges in the outer loop
condition if you prefer), leaving the existing loops and key_count logic
unchanged.
| sds err = NULL; | ||
| if (validateSlotRanges(slots, &err) != C_OK) { | ||
| addReplyErrorSds(c, err); | ||
| if (slotRangeArrayNormalizeAndValidate(slots, &err) != C_OK) { | ||
| sdsfree(err); | ||
| slotRangeArrayFree(slots); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
Validation error message is discarded without being sent to the client.
The function name parseSlotRangesOrReply suggests it should reply with errors, but the validation error from slotRangeArrayNormalizeAndValidate is freed without sending it to the client. The caller receives NULL with no explanation.
🐛 Proposed fix
sds err = NULL;
if (slotRangeArrayNormalizeAndValidate(slots, &err) != C_OK) {
+ addReplyErrorSds(c, err);
- sdsfree(err);
slotRangeArrayFree(slots);
return NULL;
}📝 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.
| sds err = NULL; | |
| if (validateSlotRanges(slots, &err) != C_OK) { | |
| addReplyErrorSds(c, err); | |
| if (slotRangeArrayNormalizeAndValidate(slots, &err) != C_OK) { | |
| sdsfree(err); | |
| slotRangeArrayFree(slots); | |
| return NULL; | |
| } | |
| sds err = NULL; | |
| if (slotRangeArrayNormalizeAndValidate(slots, &err) != C_OK) { | |
| addReplyErrorSds(c, err); | |
| slotRangeArrayFree(slots); | |
| return NULL; | |
| } |
🤖 Prompt for AI Agents
In `@src/cluster.c` around lines 2014 - 2019, The validation error string returned
in err from slotRangeArrayNormalizeAndValidate is being freed and discarded
instead of being sent back to the client; update parseSlotRangesOrReply so when
slotRangeArrayNormalizeAndValidate(...) != C_OK it calls the client reply API
(e.g., addReplyError(c, err) or addReplyErrorFormat using the sds) before
freeing err and slotRangeArrayFree(slots), then return NULL; this ensures the
caller/client receives the validation message.
2 participants
Benchmark PR from agentic-review-benchmarks#2
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.