From 53007f46f57896a1002c797657e91a3d54404729 Mon Sep 17 00:00:00 2001 From: Pavel Kalinnikov Date: Wed, 7 Feb 2024 12:58:44 +0000 Subject: [PATCH 1/3] tracker: maintain Match < Next invariant 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 --- confchange/confchange.go | 2 +- confchange/testdata/joint_autoleave.txt | 6 +++--- confchange/testdata/joint_idempotency.txt | 6 +++--- confchange/testdata/joint_learners_next.txt | 6 +++--- confchange/testdata/simple_idempotency.txt | 10 +++++----- confchange/testdata/simple_promote_demote.txt | 14 +++++++------- confchange/testdata/update.txt | 6 +++--- confchange/testdata/zero.txt | 2 +- tracker/progress.go | 11 +++++++++-- 9 files changed, 35 insertions(+), 28 deletions(-) diff --git a/confchange/confchange.go b/confchange/confchange.go index 55db16a8..a73d5609 100644 --- a/confchange/confchange.go +++ b/confchange/confchange.go @@ -259,8 +259,8 @@ func (c Changer) initProgress(cfg *tracker.Config, trk tracker.ProgressMap, id u // at all (and will thus likely need a snapshot), though the app may // have applied a snapshot out of band before adding the replica (thus // making the first index the better choice). - Next: c.LastIndex, Match: 0, + Next: max(c.LastIndex, 1), // invariant: Match < Next Inflights: tracker.NewInflights(c.Tracker.MaxInflight, c.Tracker.MaxInflightBytes), IsLearner: isLearner, // When a node is first added, we should mark it as recently active. diff --git a/confchange/testdata/joint_autoleave.txt b/confchange/testdata/joint_autoleave.txt index 9ec8cb0a..be138df3 100644 --- a/confchange/testdata/joint_autoleave.txt +++ b/confchange/testdata/joint_autoleave.txt @@ -5,14 +5,14 @@ simple v1 ---- voters=(1) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 # Autoleave is reflected in the config. enter-joint autoleave=true v2 v3 ---- voters=(1 2 3)&&(1) autoleave -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 2: StateProbe match=0 next=1 3: StateProbe match=0 next=1 @@ -24,6 +24,6 @@ config is already joint leave-joint ---- voters=(1 2 3) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 2: StateProbe match=0 next=1 3: StateProbe match=0 next=1 diff --git a/confchange/testdata/joint_idempotency.txt b/confchange/testdata/joint_idempotency.txt index 6d1346b7..a47f3a66 100644 --- a/confchange/testdata/joint_idempotency.txt +++ b/confchange/testdata/joint_idempotency.txt @@ -5,19 +5,19 @@ simple v1 ---- voters=(1) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 enter-joint r1 r2 r9 v2 v3 v4 v2 v3 v4 l2 l2 r4 r4 l1 l1 ---- voters=(3)&&(1) learners=(2) learners_next=(1) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 2: StateProbe match=0 next=1 learner 3: StateProbe match=0 next=1 leave-joint ---- voters=(3) learners=(1 2) -1: StateProbe match=0 next=0 learner +1: StateProbe match=0 next=1 learner 2: StateProbe match=0 next=1 learner 3: StateProbe match=0 next=1 diff --git a/confchange/testdata/joint_learners_next.txt b/confchange/testdata/joint_learners_next.txt index df1da7d0..6faddfe7 100644 --- a/confchange/testdata/joint_learners_next.txt +++ b/confchange/testdata/joint_learners_next.txt @@ -8,17 +8,17 @@ simple v1 ---- voters=(1) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 enter-joint v2 l1 ---- voters=(2)&&(1) learners_next=(1) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 2: StateProbe match=0 next=1 leave-joint ---- voters=(2) learners=(1) -1: StateProbe match=0 next=0 learner +1: StateProbe match=0 next=1 learner 2: StateProbe match=0 next=1 diff --git a/confchange/testdata/simple_idempotency.txt b/confchange/testdata/simple_idempotency.txt index 2f7ca2e2..e31c43b7 100644 --- a/confchange/testdata/simple_idempotency.txt +++ b/confchange/testdata/simple_idempotency.txt @@ -2,33 +2,33 @@ simple v1 ---- voters=(1) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 simple v1 ---- voters=(1) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 simple v2 ---- voters=(1 2) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 2: StateProbe match=0 next=2 simple l1 ---- voters=(2) learners=(1) -1: StateProbe match=0 next=0 learner +1: StateProbe match=0 next=1 learner 2: StateProbe match=0 next=2 simple l1 ---- voters=(2) learners=(1) -1: StateProbe match=0 next=0 learner +1: StateProbe match=0 next=1 learner 2: StateProbe match=0 next=2 simple diff --git a/confchange/testdata/simple_promote_demote.txt b/confchange/testdata/simple_promote_demote.txt index 52369b45..b4b770de 100644 --- a/confchange/testdata/simple_promote_demote.txt +++ b/confchange/testdata/simple_promote_demote.txt @@ -4,20 +4,20 @@ simple v1 ---- voters=(1) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 simple v2 ---- voters=(1 2) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 2: StateProbe match=0 next=1 simple v3 ---- voters=(1 2 3) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 2: StateProbe match=0 next=1 3: StateProbe match=0 next=2 @@ -27,7 +27,7 @@ simple l1 v1 ---- voters=(1 2 3) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 2: StateProbe match=0 next=1 3: StateProbe match=0 next=2 @@ -36,7 +36,7 @@ simple l2 ---- voters=(1 3) learners=(2) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 2: StateProbe match=0 next=1 learner 3: StateProbe match=0 next=2 @@ -46,7 +46,7 @@ simple v2 l2 ---- voters=(1 3) learners=(2) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 2: StateProbe match=0 next=1 learner 3: StateProbe match=0 next=2 @@ -55,6 +55,6 @@ simple v2 ---- voters=(1 2 3) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 2: StateProbe match=0 next=1 3: StateProbe match=0 next=2 diff --git a/confchange/testdata/update.txt b/confchange/testdata/update.txt index 50a703cc..ac47bf3e 100644 --- a/confchange/testdata/update.txt +++ b/confchange/testdata/update.txt @@ -6,18 +6,18 @@ simple v1 ---- voters=(1) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 simple v2 u1 ---- voters=(1 2) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 2: StateProbe match=0 next=1 simple u1 u2 u3 u1 u2 u3 ---- voters=(1 2) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 2: StateProbe match=0 next=1 diff --git a/confchange/testdata/zero.txt b/confchange/testdata/zero.txt index 5e0d46fe..226ade08 100644 --- a/confchange/testdata/zero.txt +++ b/confchange/testdata/zero.txt @@ -3,4 +3,4 @@ simple v1 r0 v0 l0 ---- voters=(1) -1: StateProbe match=0 next=0 +1: StateProbe match=0 next=1 diff --git a/tracker/progress.go b/tracker/progress.go index 32c5ee24..40041730 100644 --- a/tracker/progress.go +++ b/tracker/progress.go @@ -28,7 +28,15 @@ import ( // strewn around `*raft.raft`. Additionally, some fields are only used when in a // certain State. All of this isn't ideal. type Progress struct { - Match, Next uint64 + // Match is the index up to which the follower's log is known to match the + // leader's. + Match uint64 + // Next is the log index of the next entry to send to this follower. All + // entries with indices in (Match, Next) interval are already in flight. + // + // Invariant: 0 <= Match < Next. + Next uint64 + // State defines how the leader should interact with the follower. // // When in StateProbe, leader sends at most one replication message @@ -208,7 +216,6 @@ func (pr *Progress) MaybeDecrTo(rejected, matchHint uint64) bool { return false } - // Next index shall always be larger than match index. pr.Next = max(min(rejected, matchHint+1), pr.Match+1) pr.MsgAppFlowPaused = false return true From 79a64f4a3bff880a6c1ce4a5b1e7d10a233511ce Mon Sep 17 00:00:00 2001 From: Pavel Kalinnikov Date: Wed, 14 Feb 2024 22:10:12 +0000 Subject: [PATCH 2/3] raft: remove unneeded MaybeUpdate call 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 --- raft.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/raft.go b/raft.go index 3357ae45..7f591f26 100644 --- a/raft.go +++ b/raft.go @@ -1902,9 +1902,6 @@ func (r *raft) restore(s pb.Snapshot) bool { assertConfStatesEquivalent(r.logger, cs, r.switchToConfig(cfg, trk)) - pr := r.trk.Progress[r.id] - pr.MaybeUpdate(pr.Next - 1) // TODO(tbg): this is untested and likely unneeded - last := r.raftLog.lastEntryID() r.logger.Infof("%x [commit: %d, lastindex: %d, lastterm: %d] restored snapshot [index: %d, term: %d]", r.id, r.raftLog.committed, last.index, last.term, id.index, id.term) From 457bb8ea975e74c61ab13d8c377ebd5ce358695d Mon Sep 17 00:00:00 2001 From: Pavel Kalinnikov Date: Wed, 14 Feb 2024 22:40:21 +0000 Subject: [PATCH 3/3] tracker: clean-up MaybeUpdate Signed-off-by: Pavel Kalinnikov --- tracker/progress.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/tracker/progress.go b/tracker/progress.go index 40041730..cb4312a9 100644 --- a/tracker/progress.go +++ b/tracker/progress.go @@ -174,14 +174,13 @@ func (pr *Progress) UpdateOnEntriesSend(entries int, bytes uint64) { // index acked by it. The method returns false if the given n index comes from // an outdated message. 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.MsgAppFlowPaused = false + if n <= pr.Match { + return false } - pr.Next = max(pr.Next, n+1) - return updated + pr.Match = n + pr.Next = max(pr.Next, n+1) // invariant: Match < Next + pr.MsgAppFlowPaused = false + return true } // MaybeDecrTo adjusts the Progress to the receipt of a MsgApp rejection. The