storage: Write HardState atomically with committing splits

[bdarnell]

Prior to this change, an ill-timed crash (between applying the raft
command and calling splitPostApply) would leave the replica in a
persistently broken state (no HardState).

Found via jepsen.

Fixes #20629
Fixes #20494

Release note (bugfix): Fixed a replica corruption that could occur if
a process crashed in the middle of a range split.

[cockroach-teamcity]

This change is 

[nvanbenschoten]

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

---

pkg/storage/replica.go, line 4615 at r1 (raw file):

			// on each replica (votes may have already been cast on the
			// uninitialized replica). Transform the write batch to add the
			// updated HardState.

Consider linking to the issue here.

---

pkg/storage/replica.go, line 4616 at r1 (raw file):

			// uninitialized replica). Transform the write batch to add the
			// updated HardState.
			tmpBatch := r.store.engine.NewBatch()

Would using NewWriteOnlyBatch make a difference here?

---

pkg/storage/replica.go, line 4617 at r1 (raw file):

			// updated HardState.
			tmpBatch := r.store.engine.NewBatch()
			if err := tmpBatch.ApplyBatchRepr(writeBatch.Data, false); err != nil {

Is there any performance concern with this BatchRepr copy? This is only done on splits, so probably not.

---

pkg/storage/store.go, line 1801 at r1 (raw file):

}

// splitPreApply is called when the raft command is applied. Any

Is there a reason why these functions are in store.go instead of with the other pre and post apply functions in replica_proposal.go?

---

Comments from Reviewable

[bdarnell]

Review status: 0 of 2 files reviewed at latest revision, 4 unresolved discussions.

---

pkg/storage/replica.go, line 4615 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Consider linking to the issue here.

Done

---

pkg/storage/replica.go, line 4616 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Would using NewWriteOnlyBatch make a difference here?

Perhaps, but that means encoding knowledge-at-a-distance that splitPreApply doesn't need to read anything else that may have been written in the batch.

---

pkg/storage/replica.go, line 4617 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is there any performance concern with this BatchRepr copy? This is only done on splits, so probably not.

Yeah, splits are uncommon enough (and small enough) that I'm not concerned. Added a comment.

---

pkg/storage/store.go, line 1801 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is there a reason why these functions are in store.go instead of with the other pre and post apply functions in replica_proposal.go?

Historical reasons (and adjacency with Store.SplitRange, from which splitPostApply was extracted). I figure we'll gather all the split-related code into one place as a part of #18779.

---

Comments from Reviewable

[nvanbenschoten]

---

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

---

Comments from Reviewable

[tbg]

---

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/storage/replica.go, line 33 at r2 (raw file):

	"github.com/google/btree"
	"github.com/kr/pretty"
	opentracing "github.com/opentracing/opentracing-go"

I remember having that problem back when I used vim.

---

Comments from Reviewable