Reviewed 3 of 3 files at r1, 7 of 7 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 5 of 5 files at r5, 6 of 6 files at r6, 1 of 1 files at r7, 8 of 8 files at r8, 12 of 12 files at r9, 21 of 21 files at r10.
Review status: all files reviewed at latest revision, 12 unresolved discussions.

^{client/db_internal_test.go, line 37 [r10] (raw file):}
include the numbers in the error

^{kv/txn_coord_sender.go, line 304 [r10] (raw file):}
has a sequence number?

^{roachpb/batch.go, line 262 [r10] (raw file):}
Hm, alright then

^{storage/client_split_test.go, line 304 [r5] (raw file):}
include err?

^{storage/client_split_test.go, line 315 [r5] (raw file):}
include err?

^{storage/engine/rocksdb/db.cc, line 186 [r2] (raw file):}
This local can be removed now

^{storage/replica.go, line 1108 [r5] (raw file):}
dont see the value in this rename

^{storage/replica.go, line 143 [r10] (raw file):}
why not make this a struct instead of encoding all the stufff below?

^{storage/replica_test.go, line 1513 [r9] (raw file):}
can't you do equality or type assertion here?

^{storage/response_cache.go, line 174 [r5] (raw file):}
The timestamp on the value is just so it matches the other argument? this function signature is pretty strange.

^{storage/response_cache.go, line 77 [r9] (raw file):}
this comment is way stale

^{storage/response_cache.go, line 91 [r9] (raw file):}
it should be mentioned somewhere that txn sequence are 1-indexed

Comments from the review on Reviewable.io

LGTM

So much code deletion!

Review status: all files reviewed at latest revision, 5 unresolved discussions.

^{storage/replica.go, line 907 [r10] (raw file):}
Why even random? Seems like you could have a sequence number inside Replica. That seems preferable to a random number for debugging purposes.

Comments from the review on Reviewable.io

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

^{kv/txn_coord_sender.go, line 304 [r10] (raw file):}
nope, tracing uses only the transaction ID.

^{storage/replica.go, line 1108 [r5] (raw file):}
if err := func(err) looks mildly confusing.

^{storage/replica.go, line 143 [r10] (raw file):}
MultiRaft uses a string internally.

^{storage/replica_test.go, line 1513 [r9] (raw file):}
no, the replica corruption error is package-internal, and this is in storage_test.

Comments from the review on Reviewable.io

added some commits addressing comments, PTAL.

Reviewed 12 of 12 files at r11, 3 of 3 files at r12, 3 of 3 files at r13, 1 of 1 files at r14.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

^{roachpb/api.go, line 54 [r13] (raw file):}
instead of a map, how about [...]string{? You'll need to put the sentinel in the array.

^{roachpb/api.go, line 506 [r13] (raw file):}
use a buffer?

Comments from the review on Reviewable.io

Review status: all files reviewed at latest revision, 3 unresolved discussions.

^{storage/replica.go, line 907 [r10] (raw file):}
Export multiraft.commandIDLen instead of hard-coding 16 here. We can also change that constant to 8 now that our command IDs are smaller.

I'm not sure if it's safe to make this a per-Replica sequence. In multiraft the ids need to be unique per multiraft.group, which mostly correspond to Replicas but have slightly different lifetimes. I'd rather keep it random.

Comments from the review on Reviewable.io

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

^{roachpb/api.go, line 54 [r13] (raw file):}
that would work for flagToStr, but it would make for a rather awkward definition (the flags are 2^i).

Comments from the review on Reviewable.io

PTAL baby one more time

LGTM

Reviewed 4 of 4 files at r15.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

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

^{storage/replica.go, line 907 [r10] (raw file):}
Done.

Comments from the review on Reviewable.io