kvserver: make logAppend and sideloading stand-alone

[tbg]

*Replica is a big bag of behavior and we should try to encapsulate the
replication layer from it as much as possible.

Methods anchored on *Replica are particularly problematic, since it's
anyone's guess which of the many members they access. Instead, the
pattern we should follow is that as many methods as possible are
stand-alone and interact with the *Replica only ways obvious from
the parameters.

It was really easy to do this for (*Replica).append and for
(*Replica).maybeSideloadEntries, and this commit does it.

Epic: CRDB-220
Release note: None

[cockroach-teamcity]

This change is 

[pav-kv]

This change LGTM, with a few meta-comments.

As things move out of Replica, the "raftMu must be locked" rule becomes harder to feel. Maybe we should rethink how to make it more apparent and less error prone at scale.

Moving stand-alone funcs is fine, but maybe we could find some bigger abstractions and factor them out of Replica as types?

The PR could be split in 2 commits, one per factored out method.

[erikgrinaker]

Moving stand-alone funcs is fine, but maybe we could find some bigger abstractions and factor them out of Replica as types?

+1. At least with struct methods, I have some sort of conceptual grouping. A bunch of random standalone functions makes it harder to understand the overall organization, at least to me.

[tbg]

Agree with both of you for the most part. The first step to an abstraction is to make all of the engine mutations standalone and the abstractions are well underway, with #91579 and #76126.

As things move out of Replica, the "raftMu must be locked" rule becomes harder to feel.

I think this is a good thing! The parts that manipulate the raft state shouldn't know about raftMu. They should only be called under raftMu. It is quite tricky to make sure mutual exclusion holds and much more is involved than just raftMu at least once we get to state application. I think it is a benefit to contain this locking in Store/Replica but not make it something that the code that implements the engine interactions to have to care about.
I agree that while we pull out ad-hoc functions it can be confusing, but once this gets grouped onto a struct or behind an abstraction I think it will feel quite natural. I will try to get #76126 merged soon and then we have a package to move things to.

bors=pavelkalinnikov

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @pavelkalinnikov)

---

pkg/kv/kvserver/replica_raftstorage.go line 633 at r1 (raw file):

Previously, pavelkalinnikov (Pavel Kalinnikov) wrote…

This now unconditionally takes ReadWriter. Were there any conditions under which an "only" Writer was passed in. There seems to be only one user of this func, so we're good?

Correct, this behavior was not needed and besides, it seems like an antipattern to take an X but to insist it can be upcast to a Y in certain cases.

[tbg]

bors r=pavelkalinnikov

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)