storage: compute quorum commit index instead of using raft.Status.Commit

[petermattis]

Compute the index at which a quorum of nodes have committed instead of using raft.Status.Commit. The latter can be in advance of the computed quorum commit index just after a replica has been added to the group. And by computing the value ourselves we can include the pending snapshot index in the computation.

Fixes #10409

---

This change is

[tamird]

---

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable

[bdarnell]

as a quick fix for the current issues, although I think there are times where we'd want to truncate the logs even at the cost of a second snapshot, just like we don't require that 100% of the current replicas are up to date. The best way to do this may be to pass the pending snapshot index into getBehindIndex, where it gets appended to the list of log positions from the current replicas (treating the pending snapshot as if it's already been added to the configuration)

---

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[petermattis]

The best way to do this may be to pass the pending snapshot index into getBehindIndex, where it gets appended to the list of log positions from the current replicas (treating the pending snapshot as if it's already been added to the configuration)

Don't we already effectively accomplish this by processing pendingSnapshotIndex after calling getBehindIndex in computeTruncatableIndex?

---

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[petermattis]

Rewrote this so that we compute the quorum commit index ourselves and include the pending snapshot index in the computation. That accomplishes the same effect as the previous incarnation of this PR. PTAL.

[tamird]

Reviewed 1 of 2 files at r2.
Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/storage/raft_log_queue.go, line 151 at r2 (raw file):

      // way that will require that new replica to be caught up via a Raft
      // snapshot.
      if pendingSnapshotIndex > 0 && truncatableIndex > pendingSnapshotIndex {

why is this still necessary? getQuorumIndex appears to take this into account now.

---

pkg/storage/raft_log_queue.go, line 159 at r2 (raw file):

      truncatableIndex = firstIndex
  }
  // Never truncate past the quorum committed index.

i think you can remove this now; it is no longer possible for this case to occur.

---

Comments from Reviewable

[petermattis]

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/storage/raft_log_queue.go, line 151 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why is this still necessary? getQuorumIndex appears to take this into account now.

There is a case in `TestComputeTruncatableIndex` which demonstrates the need. The `quorumIndex` might be greater than `pendingSnapshotIndex` if `pendingSnapshotIndex` is the furthest behind. We still want to maintain the raft log for the `pendingSnapshotIndex` if the raft log is small enough.

---

pkg/storage/raft_log_queue.go, line 159 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

i think you can remove this now; it is no longer possible for this case to occur.

I don't think it could have occurred before. I think of this more as a sanity check than a required bit of code.

---

Comments from Reviewable

[tamird]

Review status: 1 of 2 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

pkg/storage/raft_log_queue.go, line 151 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

There is a case in TestComputeTruncatableIndex which demonstrates the need. The quorumIndex might be greater than pendingSnapshotIndex if pendingSnapshotIndex is the furthest behind. We still want to maintain the raft log for the pendingSnapshotIndex if the raft log is small enough.

I see. This formula is pretty confusing; all this code wants is the minimum of a bunch of `uint64`s, naively:

a := uint64Slice{quorumIndex}
if pendingSnapshotIndex >0 {
  a = append(a, pendingSnapshoIndex)
}
for _, p := range raftStatus.Progress {
  a = append(a, p.Match)
}
sort.Sort(a)
idx := a[0]

The quantity of functions and control flow employed here to express this outcome bizarrely outweighs the conceptual complexity. I guess I'm arguing for the removal of getBehindindex as that method does nothing to enhance readability though it does currently have unit test coverage (which are also of dubious value compared to tests of getTruncatableIndex).

---

pkg/storage/raft_log_queue.go, line 159 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I don't think it could have occurred before. I think of this more as a sanity check than a required bit of code.

OK. I find this pretty confusing, but I won't insist.

---

pkg/storage/raft_log_queue.go, line 166 at r2 (raw file):

}

// getBehindIndex returns the raft log index of the oldest node or the quorum

this comment is kind of odd now; this function doesn't compute the quorum commit index, it just trusts the caller. Perhaps a more accurate phrasing would be "returns the minimum of the raft log indices and the supplied quorum index".

---

Comments from Reviewable

[petermattis]

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/storage/raft_log_queue.go, line 151 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I see. This formula is pretty confusing; all this code wants is the minimum of a bunch of uint64s, naively:

a := uint64Slice{quorumIndex}
if pendingSnapshotIndex >0 {
  a = append(a, pendingSnapshoIndex)
}
for _, p := range raftStatus.Progress {
  a = append(a, p.Match)
}
sort.Sort(a)
idx := a[0]

The quantity of functions and control flow employed here to express this outcome bizarrely outweighs the conceptual complexity. I guess I'm arguing for the removal of getBehindindex as that method does nothing to enhance readability though it does currently have unit test coverage (which are also of dubious value compared to tests of getTruncatableIndex).

What you have is functionality equivalent to the existing code, though slower because it builds up a slice. I'm sure there are various ways we can make this code more elegant. Do you really think we should spend time doing so? It's not like the previous code was buggy because if its structure. It was working exactly as advertised.

---

pkg/storage/raft_log_queue.go, line 166 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this comment is kind of odd now; this function doesn't compute the quorum commit index, it just trusts the caller. Perhaps a more accurate phrasing would be "returns the minimum of the raft log indices and the supplied quorum index".

Done.

---

Comments from Reviewable

[tamird]

Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/storage/raft_log_queue.go, line 151 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

What you have is functionality equivalent to the existing code, though slower because it builds up a slice. I'm sure there are various ways we can make this code more elegant. Do you really think we should spend time doing so? It's not like the previous code was buggy because if its structure. It was working exactly as advertised.

Yes, it is slower - that's why I used the word "naively". Do I think we should clarify this code? Well, yes, since the cognitive overhead of parsing it is a tax paid on every visit.

Such a simplification shouldn't hold up this change (I've already approved it), but I mention it here because you (and now I) have already paid the parsing tax.

---

pkg/storage/raft_log_queue_test.go, line 63 at r3 (raw file):

}

func TestGetQuorumIndex(t *testing.T) {

as with TestGetBehindIndex, I am not sure this test provides much value; it is explicitly a subset of TestComputeTruncatableIndex

---

Comments from Reviewable

[petermattis]

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/storage/raft_log_queue.go, line 151 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Yes, it is slower - that's why I used the word "naively". Do I think we should clarify this code? Well, yes, since the cognitive overhead of parsing it is a tax paid on every visit.

Such a simplification shouldn't hold up this change (I've already approved it), but I mention it here because you (and now I) have already paid the parsing tax.

Ok, I inlined `getBehindIndex`.

---

pkg/storage/raft_log_queue_test.go, line 63 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

as with TestGetBehindIndex, I am not sure this test provides much value; it is explicitly a subset of TestComputeTruncatableIndex

True, but unlike `getBehindIndex`, `getQuorumIndex` is a bit more involved and worth testing independently IMO.

---

Comments from Reviewable

[tamird]

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/storage/raft_log_queue.go, line 151 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Ok, I inlined getBehindIndex.

Thanks.

---

pkg/storage/raft_log_queue.go, line 144 at r4 (raw file):

  if raftLogSize <= targetSize {
      // Only truncate to one of the behind indexes if the raft log is less than

s/behind/follower/?

---

Comments from Reviewable

[petermattis]

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion.

---

pkg/storage/raft_log_queue.go, line 144 at r4 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

s/behind/follower/?

Done.

---

Comments from Reviewable

[tamird]

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable