Refactor some of ASM and slot-stats functions#35
Conversation
Review Summary by QodoRefactor cluster initialization and ASM integration across implementations
WalkthroughsDescription• Refactor cluster initialization to separate common and legacy-specific logic • Move ASM (Atomic Slot Migration) calls to server cron and before-sleep hooks • Extract slot verification logic into reusable functions for different cluster implementations • Add support for migration and syncslots subcommands in cluster.c Diagramflowchart LR
A["Cluster Init"] --> B["clusterCommonInit"]
B --> C["Initialize Slot Stats"]
B --> D["Reset Cluster Stats"]
B --> E["ASM Init"]
F["Server Cron"] --> G["ASM Cron"]
G --> H["Cluster Cron"]
I["Before Sleep"] --> J["Cluster Before Sleep"]
J --> K["ASM Before Sleep"]
L["Verify Config"] --> M["Delete Unowned Slots"]
L --> N["Claim Unassigned Slots"]
File Changes1. src/cluster.c
|
Code Review by Qodo
1. clusterCommonInit() uses malloc
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| /* This function is called at server startup in order to initialize cluster data | ||
| * structures that are shared between the different cluster implementations. */ | ||
| void clusterCommonInit(void) { | ||
| server.cluster_slot_stats = malloc(CLUSTER_SLOTS*sizeof(clusterSlotStat)); |
There was a problem hiding this comment.
1. clustercommoninit() uses malloc 📘 Rule violation ⛯ Reliability
The new clusterCommonInit() allocates server.cluster_slot_stats via malloc(), bypassing Redis memory tracking. This violates the requirement to use the zmalloc/zfree family for allocations in src/.
Agent Prompt
## Issue description
`clusterCommonInit()` allocates memory with `malloc()`, which bypasses Redis's memory tracking and violates the requirement to use the `zmalloc`/`zfree` family in `src/`.
## Issue Context
`server.cluster_slot_stats` was previously allocated via `zmalloc()` in `main()`, but that allocation was replaced by a call to `clusterCommonInit()`, which now uses `malloc()`.
## Fix Focus Areas
- src/cluster.c[2137-2141]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| /* This function is called after the node startup in order to check if there | ||
| * are any slots that we have keys for, but are not assigned to us. If so, | ||
| * we delete the keys. */ | ||
| void clusterDeleteKeysInUnownedSlots(void) { |
There was a problem hiding this comment.
2. clusterdeletekeysinunownedslots() not static 📘 Rule violation ✓ Correctness
The new helper clusterDeleteKeysInUnownedSlots() is defined without static, even though it appears to be an internal-only function used within the same source file. This unnecessarily exports a global symbol and violates the encapsulation requirement.
Agent Prompt
## Issue description
`clusterDeleteKeysInUnownedSlots()` is an internal helper but is declared as a global (non-`static`) function, which violates the requirement to keep internal helpers `static` to avoid symbol pollution.
## Issue Context
This function is newly introduced in `src/cluster.c` as part of refactoring the cluster startup verification flow and is intended as an internal helper.
## Fix Focus Areas
- src/cluster.c[2146-2168]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+2209
to
+2212
| /* Delete keys in unowned slots */ | ||
| clusterDeleteKeysInUnownedSlots(); | ||
| /* Take over slots that we have keys for, but are assigned to no one. */ | ||
| clusterClaimUnassignedSlots(); |
There was a problem hiding this comment.
3. Unassigned slot keys deleted 🐞 Bug ✓ Correctness
verifyClusterConfigWithData() calls clusterDeleteKeysInUnownedSlots() before clusterClaimUnassignedSlots(), but the delete routine does not exclude unassigned slots, so keys loaded from disk for unassigned slots can be deleted before they can be claimed. This causes irreversible data loss at startup for nodes that have keys in slots with no owner in the cluster config.
Agent Prompt
### Issue description
`verifyClusterConfigWithData()` currently deletes keys in slots that are not "my slot" before attempting to claim unassigned slots. Because `clusterDeleteKeysInUnownedSlots()` does not exclude unassigned slots, keys in unassigned slots can be deleted and then cannot be claimed.
### Issue Context
- Unassigned slot => `getNodeBySlot(slot)` is `NULL`, so `clusterIsMySlot(slot)` is false.
- `clusterClaimUnassignedSlots()` only claims when the slot is unassigned *and* still has keys.
### Fix Focus Areas
- src/cluster.c[2146-2168]
- src/cluster.c[2209-2213]
- src/cluster_legacy.c[5262-5284]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Benchmark PR from agentic-review-benchmarks#4