Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tracker: clean-up Match, Next and MaybeUpdate #165

Merged
merged 3 commits into from
Feb 21, 2024

Conversation

pav-kv
Copy link
Contributor

@pav-kv pav-kv commented Feb 14, 2024

This PR documents the meaning of Match and Next field in the Progress struct. It also ensures that the Match < Next invariant is maintained in all code places that initialize or modify these fields.

It then simplifies the Progress.MaybeUpdate method, given that the invariant is true before it is invoked.

@pav-kv pav-kv force-pushed the maybe-update branch 4 times, most recently from f98f6df to a87b2e7 Compare February 14, 2024 23:04
@pav-kv pav-kv requested a review from ahrtr February 14, 2024 23:07
@pav-kv
Copy link
Contributor Author

pav-kv commented Feb 14, 2024

@ahrtr PTAL

Match: 0,
Next: max(c.LastIndex, 1), // invariant: Match < Next
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the Next is set to lastIndex + 1 in reset? Should we make them consistent?

raft/raft.go

Line 784 in 83d8dec

Next: r.raftLog.lastIndex() + 1,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried c.LastIndex + 1 here for consistency, but it breaks a few tests. I think the reason is: setting to lastIndex + 1 precludes sending a MsgApp message instantly (the maybeSendAppend treats it as "everything is in flight", and won't send any appends until there is a heartbeat reply). So, here we set it to LastIndex to guarantee at least one message send. Also, typically this last entry contains the very config change that we're performing here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setting to lastIndex + 1 precludes sending a MsgApp message instantly

Makes sense.

@@ -115,17 +123,17 @@ func (pr *Progress) BecomeProbe() {
if pr.State == StateSnapshot {
pendingSnapshot := pr.PendingSnapshot
pr.ResetState(StateProbe)
pr.Next = max(pr.Match+1, pendingSnapshot+1)
pr.Next = max(pr.Match+1, pendingSnapshot+1) // invariant: Match < Next
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see there are comments "// invariant: Match < Next" in multiple places. I think we only need to keep the comment in type Progress struct, and also add verification (I was planning to do it for a while) to safeguard it instead of adding comment everywhere.

Copy link
Contributor Author

@pav-kv pav-kv Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the comment from all the places, except the non-trivial max(c.LastIndex, 1) in one place (where this invariant was not maintained before this PR). It should be ok to have comments like this though, IMO, esp. while the invariant is not checked explicitly at the moment.

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with a minor comment

cc @erikgrinaker

This commit documents the meaning of Match and Next field in the
Progress struct. It also ensures that the invariant is maintained in all
code places that initialize or modify these fields.

Signed-off-by: Pavel Kalinnikov <[email protected]>
The Progress.MaybeUpdate is only needed on the leader. In the
raft.restore() method, the state is either Follower or Candidate.
Non-leader states don't have a replication flow, and don't need to track
Match/Next index.

Signed-off-by: Pavel Kalinnikov <[email protected]>
Signed-off-by: Pavel Kalinnikov <[email protected]>
@erikgrinaker
Copy link
Contributor

I won't have time to look at this until next week.

Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, sorry for the holdup.

@ahrtr ahrtr merged commit d475d7e into etcd-io:main Feb 21, 2024
10 checks passed
@pav-kv pav-kv deleted the maybe-update branch February 21, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants