This change is 

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

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

// evaluating it. The returned ProposalData is partially valid even
// on a non-nil *roachpb.Error and should be proposed through Raft
// if ProposalData.command is non-nil.

Perhaps take a moment to clarify that a non-nil ProposalData.command implies that no error was returned.

Ah, I see you have a comment to this effect in evaluateProposal. I suppose that's ok then.

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

	res, needConsensus, pErr := r.evaluateProposal(ctx, idKey, ba, spans)

	// Fill out the results even if pErr != nil; we'll return the error below.

l

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

	// following conditions is true:
	// 1. the request created a non-empty write batch.
	// 2. the request had an impact on the MVCCStats.

2 sounds like it would imply 1, though the stats fixing is likely the exception. Name that example, I think it's illuminating.

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

			EndTxns: endTxns,
		}
		proposal.finishRaftApplication(pr)

Make sure to update the comment on finishRaftApplication to make sure it's clear that this isn't always called from the Raft goroutine. Or rename it appropriately.

pkg/storage/replica_test.go, line 8723 at r1 (raw file):

			name: "delete req",
			req:  deleteReq,
			// NB: a tombstone is written even if no value exists at the key.

Good to know.

pkg/storage/engine/batch.go, line 117 at r1 (raw file):
Use the comment from the interface:

// Empty returns whether the batch is empty or not.

Comments from Reviewable

TFTR!

Review status: 0 of 9 files reviewed at latest revision, 6 unresolved discussions.

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

Did you mean to say something here?

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

Done.

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

Done.

pkg/storage/replica_test.go, line 8723 at r1 (raw file):

Yeah, I knew you'd like that one 😃

pkg/storage/engine/batch.go, line 117 at r1 (raw file):

This isn't actually an implementation of the interface, so I went with a comment that matched the other comments here.

Comments from Reviewable

Reviewed 1 of 9 files at r2, 5 of 8 files at r3, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks pending.

Comments from Reviewable

Reviewed 9 of 9 files at r2, 8 of 8 files at r3, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

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

No, my track pad is broken and randomly clicks and if that happens while I'm typing a comment I let reviewable do things I can't trace after.

pkg/storage/engine/batch.go, line 117 at r1 (raw file):

Comment here looks the same, but it's only a nit anyway. I mostly thought this sentence sounded odd.

Comments from Reviewable

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

pkg/storage/engine/batch.go, line 117 at r1 (raw file):

Done.

Comments from Reviewable

bors r=tschottdorf

Build succeeded

• GitHub CI (Cockroach)