Refactor some of ASM and slot-stats functions#26
Open
Conversation
Code Review by Qodo
1. clusterDeleteKeysInUnownedSlots() not static
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| /* 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.
1. clusterdeletekeysinunownedslots() not static 📘 Rule violation ⛯ Reliability
• clusterDeleteKeysInUnownedSlots() is defined without static even though it appears to be an internal helper used only within src/cluster.c. • Leaving it non-static unnecessarily exports the symbol and increases the chance of symbol collisions or unintended external use.
Agent prompt
## Issue description
`clusterDeleteKeysInUnownedSlots()` is an internal helper but is exported as a global symbol because it is not declared `static`.
## Issue Context
This function is used as a helper from within `src/cluster.c` (e.g., by `verifyClusterConfigWithData()`). It should not be externally visible.
## Fix Focus Areas
- src/cluster.c[2143-2214]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+2146
to
+2168
| void clusterDeleteKeysInUnownedSlots(void) { | ||
| if (clusterNodeIsSlave(getMyClusterNode())) return; | ||
|
|
||
| /* Check that all the slots we have keys for are assigned to us. Otherwise, | ||
| * delete the keys. */ | ||
| for (int i = 0; i < CLUSTER_SLOTS; i++) { | ||
| /* Skip if: no keys in the slot, it's our slot, or we are importing it. */ | ||
| if (!countKeysInSlot(i) || | ||
| clusterIsMySlot(i) || | ||
| getImportingSlotSource(i)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| serverLog(LL_NOTICE, "I have keys for slot %d, but the slot is " | ||
| "assigned to another node. " | ||
| "Deleting keys in the slot.", i); | ||
| /* With atomic slot migration, it is safe to drop keys from slots | ||
| * that are not owned. This will not result in data loss under the | ||
| * legacy slot migration approach either, since the importing state | ||
| * has already been persisted in node.conf. */ | ||
| clusterDelKeysInSlot(i, 0); | ||
| } |
There was a problem hiding this comment.
2. Deletes unassigned-slot keys 🐞 Bug ✓ Correctness
• clusterDeleteKeysInUnownedSlots() deletes keys for any slot that has keys, is not “my slot”, and is not importing; this also matches slots with no owner (unassigned), so keys in unassigned slots can be deleted at startup. • verifyClusterConfigWithData() then calls clusterClaimUnassignedSlots(), but it can no longer claim those slots because the keys were deleted. • Impact: silent data loss on restart when node.conf lacks slot ownership but DB0 contains keys for those slots.
Agent prompt
### Issue description
`clusterDeleteKeysInUnownedSlots()` currently deletes keys for slots that have keys, are not owned by this node, and are not importing. This condition also matches *unassigned* slots (owner == NULL), which means keys in unassigned slots can be deleted before `clusterClaimUnassignedSlots()` gets a chance to take ownership.
### Issue Context
This runs during `verifyClusterConfigWithData()` after loading data, so it can cause silent data loss at startup.
### Fix Focus Areas
- src/cluster.c[2146-2168]
- src/cluster.c[2209-2213]
- src/cluster.c[1722-1724]
- src/cluster_legacy.c[5266-5283]
- src/cluster_legacy.c[6499-6501]
### Implementation notes
- Add an explicit `if (getNodeBySlot(i) == NULL) continue;` (or equivalent helper) before deleting.
- Consider swapping the order in `verifyClusterConfigWithData()` to claim first, then delete unowned (owned-by-other) slots.
- Update the log text to avoid claiming "assigned to another node" when the slot is actually unassigned.
ⓘ 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