Slight simplification of configuration rollback logic #409
Conversation
Codecov Report
@@ Coverage Diff @@
## master #409 +/- ##
==========================================
+ Coverage 76.44% 76.55% +0.11%
==========================================
Files 50 50
Lines 9581 9543 -38
Branches 2459 2440 -19
==========================================
- Hits 7324 7306 -18
+ Misses 1173 1163 -10
+ Partials 1084 1074 -10
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
This new function can be used to fetch the most recent committed configuration, either from the log or from the last snapshot. Signed-off-by: Free Ekanayaka <[email protected]>
We need to get the last committed configuration in two cases: - when aborting a pending configuration change (we'll rollback to the last committed configuration) - when taking a snapshot (we include the last committed configuration in the snapshot and save a copy of it) Use the membershipFetchLastCommittedConfiguration() helper instead of duplicating the logic. Signed-off-by: Free Ekanayaka <[email protected]>
Every time we restore a snapshot we must also cache in r->configuration_previous the configuration contained in the snapshot (which the last known committed configuration at the time the snapshot was taken). This is necessary in case the log gets truncated and loses track of the entry that originally contained that configuration. We restore a snapshot in two cases: - when receiving a SnapshotInstall RPC - when starting up Instead of duplicating the caching logic in both places, move the caching logic to the snapshotRestore() utility, which is responsibile for all bookkeeping needed upon snapshot restore, no matter when and where it happens. Signed-off-by: Free Ekanayaka <[email protected]>
When we start up we first restore the last persisted snapshot (if any) and then all persisted entries. As part of resting the last persisted snapshot, we cache its associated configuration in r->configuration_previous, for rollbacks. We don't need to cache it again in restoreConfigurations(), which is called when persisted entries are restored. Signed-off-by: Free Ekanayaka <[email protected]>
After successfully taking and persisting a snapshot, we generally cache into r->configuration_previous the configuration that was included in the snapshot. Technically speaking we don't need to do that if between the time the snapshot was started and the time it completed new configuration changes were committed (those won't be truncated yet). However, for consistency, change the code to populate r->configuration_previous no matter what, so we preserve the invariant that r->configuration_previous always contains a copy of the configuration included in the most recent snapshot. Also, make the code slightly less indirect by using configurationCopy() instead of the configurationBackup() helper, which has now just this one call site and isn't worth be kept (it will be removed in a follow-up commit). Signed-off-by: Free Ekanayaka <[email protected]>
The functionality of configurationBackup() is now part of takeSnapshotCb(), and the one of configurationRestorePrevious() is now part of snapshotRestore(). Signed-off-by: Free Ekanayaka <[email protected]>
Make the logic for restoring the last configuration in the log slightly lighter, by using r->configuration_index to store the second-to-last configuration entry, instead of passing around dedicated variables. Signed-off-by: Free Ekanayaka <[email protected]>
The name "configuration_previous" is a slightly ambiguous, because this field does not actually store the "previous configuration", but rather a copy of the configuration contained in the most recent snapshot. Signed-off-by: Free Ekanayaka <[email protected]>
|
Thanks, always happy to see some redundancy removed! To be clear, r->configuration_index still refers to the index of the last committed configuration whether or not it's part of a snapshot, right? |
We might even consider renaming |
I'd support that! |
Ok then, I'll make a separate PR for that. |
This change removes a bit of code duplication in the logic that handles configuration rollbacks when the last committed configuration is not available anymore in the log and must be retrieved from the last snapshot.
It basically makes the code reflect a bit more this comment from @cole-miller that was made when the fix for configuration rollbacks was implemented.
Please see the commit messages of the individual commits in the PR for more details.
The diff removes more code than it adds, yet test coverage should slightly increase due to reduced code duplication.