raft: fix restoring joint configurations #11003
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11003 +/- ##
==========================================
- Coverage 64% 63.95% -0.05%
==========================================
Files 401 402 +1
Lines 37581 37621 +40
==========================================
+ Hits 24053 24061 +8
- Misses 11916 11943 +27
- Partials 1612 1617 +5
Continue to review full report at Codecov.
|
raft/raft.go
Outdated
|
|
||
| // applyConfChangePost is called from restore and applyConfChange to update any |
There was a problem hiding this comment.
"from newRaft, restore, and applyConfChange"
Also, I don't think you meant to say "update any updates". Did you mean process?
There was a problem hiding this comment.
I'm also not in love with this name. It doesn't really tell me anything. It would be more effective if it mentioned what it does.
There was a problem hiding this comment.
There was lots to not love about this. Now this method is in charge of actually setting the config. This turned out nicely I think, PTAL
raft/confchange/restore.go
Outdated
| } | ||
|
|
||
| // Restore takes a Changer (which must represent an empty configuration), and | ||
| // brings returns the state described by the supplied ConfState. |
raft/confchange/restore.go
Outdated
|
|
||
| var ops []func(Changer) (tracker.Config, tracker.ProgressMap, error) | ||
|
|
||
| // First, apply all of the changes of the outgoing config one by one, so that |
There was a problem hiding this comment.
Move this into the if len(outgoing) > 0 { block.
There was a problem hiding this comment.
Done. I also added additional commentary.
raft/confchange/restore.go
Outdated
| } | ||
|
|
||
| // Restore takes a Changer (which must represent an empty configuration), and | ||
| // brings returns the state described by the supplied ConfState. |
raft/confchange/restore.go
Outdated
|
|
||
| var ops []func(Changer) (tracker.Config, tracker.ProgressMap, error) | ||
|
|
||
| // First, apply all of the changes of the outgoing config one by one, so that |
There was a problem hiding this comment.
Done. I also added additional commentary.
raft/raft.go
Outdated
|
|
||
| // applyConfChangePost is called from restore and applyConfChange to update any |
There was a problem hiding this comment.
There was lots to not love about this. Now this method is in charge of actually setting the config. This turned out nicely I think, PTAL
tbg
force-pushed
the
interaction/restore
branch
from
4b95c8c to
4697813
Compare
While writing interaction tests for joint configuration changes, I realized that this wasn't working yet - restoring had no notion of the joint configuration and was simply dropping it on the floor. This commit introduces a helper `confchange.Restore` which takes a `ConfState` and initializes a `Tracker` from it. This is then used both in `(*raft).restore` as well as in `newRaft`.
tbg
force-pushed
the
interaction/restore
branch
from
4697813 to
37ab5bd
Compare
While writing interaction tests for joint configuration changes, I realized
that this wasn't working yet - restoring had no notion of the joint
configuration and was simply dropping it on the floor.
This commit introduces a helper
confchange.Restorewhich takes aConfStateand initializes aTrackerfrom it.This is then used both in
(*raft).restoreas well as innewRaft.