CASSANDRA-21663 Failed force repair should clear force repair flag - #5125
tommystendahl wants to merge 4 commits into
Conversation
tommystendahl
changed the title
00452f6 to
9d8298e
Compare
patch by Tommy Stendahl; reviewed by for CASSANDRA-21663
9d8298e to
0dd16fe
Compare
| // (the timestamp of the last SUCCESSFUL repair, which drives min_repair_interval). On | ||
| // failure this releases the flag so the node stops bypassing min_repair_interval and | ||
| // re-running repair every cycle, while the "last successful repair" time stays truthful | ||
| // instead of being bumped to now. |
There was a problem hiding this comment.
Clearing only force_repair leaves repair_start_ts > repair_finish_ts and repair_turn=MY_TURN_FORCE_REPAIR. Then when myTurnToRunRepair() runs it sees an ongoing repair and returns the forced turn, causing another repair. Nodes with an old or absent finish timestamp can still retry every cycle, so we need to mark the failure path attempt as no longer ongoing and also without recording it as successful.
The tests cover clearForceRepair() directly but don't inject a failure through AutoRepair.repair(), so they don't detect this.
There was a problem hiding this comment.
Right, I have changed so on a failed force repair repair_start_ts will be set to the last repair_finish_ts. That should stop it from being ongoing while preserve the repair_finish_ts from the last successful repair.
| // failure this releases the flag so the node stops bypassing min_repair_interval and | ||
| // re-running repair every cycle, while the "last successful repair" time stays truthful | ||
| // instead of being bumped to now. | ||
| if (forceRepairTurn) |
There was a problem hiding this comment.
if setForceRepair is called while a repair is running, this will clear it
There was a problem hiding this comment.
Yes, I didn't consider that. When I look at this the old behavior is that any successful repair clears forceRepair.
But I think that it is safe to just clear forceRepair the first thing we do when starting a force repair.
| if (forceRepairTurn) | ||
| { | ||
| continue; | ||
| AutoRepairUtils.clearForceRepair(repairType, myId); |
There was a problem hiding this comment.
If we do this here and call updateStartAutoRepairHistory later, if C* crashes after clearing the flag then there is no indication force repair should resume and it will also look like a normal repair. I think we need to pass forceTurn as a param to updateStartAutoRepairHistory so it can decide to write a normal or forced repair history statement. There we can also add a function that checks the flag for a current force, and another that checks the history for an ongoing force repair so we can detect both states.
There was a problem hiding this comment.
I moved clearing the force repair flag into updateStartAutoRepairHistory and also fixed AutoRepairHistory so it detects ongoing force repairs correctly.
| @VisibleForTesting | ||
| boolean shouldSkipRepairDueToInterval(AutoRepairConfig.RepairType repairType, AutoRepairState repairState, AutoRepairConfig config, UUID myId) | ||
| { | ||
| if (AutoRepairUtils.isForceRepairSetForNode(repairType, myId)) |
There was a problem hiding this comment.
We need to also check hasOngoingForceRepair here or crashed force repairs won't bypass min_repair_interval. The test doesn't catch this because it only checks myTurnToRunRepair.
2 participants
patch by Tommy Stendahl; reviewed by for CASSANDRA-21663