raft: extract 'tracker' package #10807
Conversation
|
@bdarnell, friendly ping. |
raft/tracker/progress.go
Outdated
| // RecentlyContacted is used in StateProbe. | ||
| // When RecentlyContacted is true, raft should pause sending replication | ||
| // message to this peer until RecentlyContacted is reset. See IsPaused(). | ||
| RecentlyContacted bool |
There was a problem hiding this comment.
RecentActive and RecentlyContacted are confusingly similar (and the fact that only one uses ly bugs me). I think the old name Paused was probably better. Or maybe something like ProbeSent to tie it to StateProbe.
There was a problem hiding this comment.
Good call, I like ProbeSent because it indicates what happens and also makes it not surprising that this is reset periodically (to send another probe).
raft/tracker/progress.go
Outdated
| // Otherwise it updates the progress and returns true. | ||
| func (pr *Progress) MaybeUpdate(n uint64) bool { | ||
| var updated bool | ||
| if pr.Match < n { | ||
| pr.Match = n | ||
| updated = true | ||
| pr.Resume() | ||
| pr.RecentlyContacted = false |
There was a problem hiding this comment.
I think inlining this assignment obscures what's going on - the contact hasn't become any less recent. What's happening is that we've received an ack to our probe. I think it would be better to indirect through a method, called either Resume as before or something like ProbeReceived().
There was a problem hiding this comment.
I inlined this because I found Resume() opaque, but I like ProbeReceived(). Actually ProbeAcked() is even better I think, going with that.
raft/tracker/state.go
Outdated
| StateProbe StateType = iota | ||
| // StateReplicate is the state steady in which a follower receives a steady |
There was a problem hiding this comment.
s/state steady/steady state/
There was a problem hiding this comment.
Done (refined the sentence a bit while I was there).
|
Thanks for the review! I just rebased and addressed your comments in fixup commits (Planning to squash the whole thing). I'll leave this open until later tomorrow so that you (or anyone else who feels like it) can give it another look. |
Codecov Report
@@ Coverage Diff @@
## master #10807 +/- ##
==========================================
+ Coverage 62.97% 63.12% +0.14%
==========================================
Files 395 398 +3
Lines 37439 37446 +7
==========================================
+ Hits 23578 23637 +59
+ Misses 12274 12231 -43
+ Partials 1587 1578 -9
Continue to review full report at Codecov.
|
Mechanically extract `progressTracker`, `Progress`, and `inflights` to their own package named `tracker`. Add lots of comments in the progress, and take the opportunity to rename and clarify various fields.
tbg
changed the title
|
Squashed, updated commit message, green. Merging. |
First four comments are #10779.
Please don't review them here.
Create a new
trackerpackage, clean up and add comments. The code movement isin the first commit and is largely mechanical, with a priority on avoiding
anything that reviewers may want to see but wouldn't spot in the midst of the
code movement.
You'll want to hold off on nits after you've also looked at the second commit
(which addresses lots of potential nits and generally tries to leave things in
a good place).
I can squash this before merging if requested.