raft: extract progress tracking into own component #10683
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10683 +/- ##
==========================================
- Coverage 71.63% 71.46% -0.18%
==========================================
Files 394 394
Lines 36658 36697 +39
==========================================
- Hits 26261 26224 -37
- Misses 8564 8627 +63
- Partials 1833 1846 +13
Continue to review full report at Codecov.
|
| @@ -1392,6 +1408,9 @@ func (r *raft) removeNode(id uint64) { | |||
| return | |||
| } | |||
|
|
|||
| // TODO(tbg): won't bad (or at least unfortunate) things happen if the | |||
There was a problem hiding this comment.
Yeah, it's not ideal. But even if the leader steps down, the nodes that remain need to elect a new leader (which could have interactions with read leases, etc). So best practice would continue to be to transfer leadership away before removing the leader. But as a fallback, having the leader step down would probably be a good idea (unless we're missing something and this is already happening).
raft/progress.go
Outdated
| // prs tracks the currently active configuration and the information known about | ||
| // the nodes and learners in it. In particular, it tracks the match index for | ||
| // each peer which in turn allows reasoning about the committed index. | ||
| type prs struct { |
There was a problem hiding this comment.
Can we give the type a better name? Maybe progressTracker?
raft/progress.go
Outdated
| pN := p.nodes[id] | ||
| pL := p.learners[id] | ||
|
|
||
| switch { |
There was a problem hiding this comment.
Just use if instead of this switch idiom.
|
@gyuho CI seems to be really flaky. Do you know why that is/is it being worked on? |
|
Regarding the failed tests: https://travis-ci.com/etcd-io/etcd/jobs/196938400#L820 https://travis-ci.com/etcd-io/etcd/jobs/196938397#L1132 https://travis-ci.com/etcd-io/etcd/jobs/196938398#L1885 |
The Progress maps contain both the active configuration and information about the replication status. By pulling it into its own component, this becomes easier to unit test and also clarifies the code, which will see changes as etcd-io#7625 is addressed. More functionality will move into `prs` in self-contained follow-up commits.
Continues what was initiated in the last commit.
This doesn't completely eliminate access to prs.nodes, but that's not really necessary. This commit uses the existing APIs in a few more places where it's convenient, and also sprinkles some assertions.
This is purely mechanical. Cleanup deferred to the next commit.
This cleans up the mechanical refactor in the last commit and will help with etcd-io#7625 as well.
It's looking at each voter's Progress and needs to know how quorums work, so this is the ideal new home for it.
This particular caller just wanted to know whether it was in a single-voter cluster configuration, which is now a question prs can answer.
This now delegates the quorum computation to r.prs, which will allow it to generalize in a straightforward way when etcd-io#7625 is addressed.
|
Haven't changed the code since the last round of reviews, and just gave myself another self-review. |
The Progress maps contain both the active configuration and information
about the replication status. By pulling it into its own component, this
becomes easier to unit test and also clarifies the code, which will see
changes as #7625 is addressed.
I broke this up into multiple commits to avoid staring at too much code
movement at once, but the bulk of the changes are still mechanical.