Reviewed 32 of 41 files at r1, 8 of 8 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/internal/client/sender.go, line 130 at r2 (raw file):

	// This is used to support historical queries (AS OF SYSTEM TIME queries and
	// backups). This method must be called on every transaction retry (but note
	// that retries should be rare for read-only queries with no clock uncertainty).

Are they possible at all?

pkg/internal/client/sender.go, line 136 at r2 (raw file):

	// and timestamp.
	// An uninitialized timestamp can be passed to leave the timestamp alone.
	ManualRestart(context.Context, roachpb.UserPriority, hlc.Timestamp)

Do we need BumpEpochAfterConcurrentRetryError if we have this?

pkg/internal/client/sender.go, line 151 at r2 (raw file):

	IsSerializablePushAndRefreshNotPossible() bool

	Epoch() uint32

This is such a large interface now that we should put some effort into ordering its methods effectively.

pkg/internal/client/sender.go, line 204 at r2 (raw file):

}

// NewMockTransactionalSender creates a MockTransactionalSender.

Make a note that the txn is cloned.

pkg/internal/client/txn.go, line 57 at r2 (raw file):

	// mu holds fields that need to be synchronized for concurrent request execution.
	mu struct {

It's surprising to me that we still have this lock. I would expect all locking to be internal to the TxnCoordSender.

pkg/internal/client/txn.go, line 61 at r2 (raw file):

		ID        uuid.UUID
		debugName string
		isolation enginepb.IsolationType

Do we need these two fields? Can't we just set it on the proto?

pkg/internal/client/txn.go, line 66 at r2 (raw file):

		// NormalUserPriority will be used.
		// TODO(andrei): Can this go away now that userPriority and Proto.Priority
		// are initialized at the same time?

I'd pull on this string now.

pkg/internal/client/txn.go, line 74 at r2 (raw file):

		// useful for catching retriable errors that have escaped inner
		// transactions, so that they don't cause a retry of an outer transaction.
		previousIDs map[uuid.UUID]struct{}

Add a TODO that this should go away when we move to a TxnAttempt model.

pkg/internal/client/txn.go, line 280 at r2 (raw file):

}

// // SetTxnAnchorKey sets the key at which to anchor the transaction record. The

TODO

pkg/internal/client/txn.go, line 759 at r2 (raw file):

// IsRetryableErrMeantForTxn returns true if err is a retryable
// error meant to restart this client transaction.
func (txn *Txn) IsRetryableErrMeantForTxn(retryErr roachpb.HandledRetryableTxnError) bool {

Can we get rid of this?

pkg/internal/client/txn.go, line 819 at r2 (raw file):

		txn.mu.Lock()
		txn.resetDeadlineLocked()
		txn.replaceSenderMaybeLocked(ctx, retryErr, requestTxnID)

"maybe locked"? I think you mean maybeReplaceSenderLocked.

pkg/internal/client/txn_test.go, line 174 at r2 (raw file):

// TestTxnRequestTxnTimestamp verifies response txn timestamp is
// always upgraded on successive requests.
func TestTxnRequestTxnTimestamp(t *testing.T) {

Did this test survive? It looks legit.

EDIT: looks like it did!

pkg/kv/txn_coord_sender.go, line 47 at r2 (raw file):

// be sent.
//go:generate stringer -type=txnState
type txnState int

Nice! I'm really glad to see these states simplified.

pkg/kv/txn_coord_sender.go, line 233 at r2 (raw file):

	ctx context.Context, ba roachpb.BatchRequest,
) (*roachpb.BatchResponse, *roachpb.Error) {
	if _, ok := ba.GetArg(roachpb.BeginTransaction); ok {

I'm not sure how I feel about this change. Seems like it would be a lot cleaner to just remove the unnecessary assertion in txnIntentCollector. It's not giving us anything and moving to something like this just so that we can enforce the assertion isn't great. It's introducing strange circular dependencies and preventing interceptors from the local reasoning that currently makes them straightforward to think about.

pkg/kv/txn_coord_sender.go, line 462 at r2 (raw file):

	tcs.interceptorStack = [...]txnInterceptor{
		&tcs.interceptorAlloc.txnHeartbeat,
		&tcs.interceptorAlloc.txnSeqNumAllocator,

This order is surprising. We don't want heartbeat requests to have sequence numbers, so I would expect the two new interceptors to be in the opposite order.

pkg/kv/txn_coord_sender.go, line 496 at r2 (raw file):

	var meta roachpb.TxnCoordMeta
	meta.Txn = tc.mu.txn.Clone()
	meta.CommandCount = tc.interceptorAlloc.txnSeqNumAllocator.commandCount

This is breaking the abstraction for no reason. See my comment about txnSeqNumAllocator.populateMetaLocked.

pkg/kv/txn_coord_sender.go, line 670 at r2 (raw file):

// Can't use linearizable mode with clockless reads since in that case we don't
// know how long to sleep - could be forever!
func (tc *TxnCoordSender) sleepForLineariazableMaybe(

maybeSleepForLinearizability

pkg/kv/txn_coord_sender.go, line 687 at r2 (raw file):

	if maxOffset != timeutil.ClocklessMaxOffset && tc.linearizable && sleepNS > 0 {
		// TODO(andrei): perhaps we shouldn't sleep with the lock held.

This is on an EndTransaction, so not a huge deal.

pkg/kv/txn_coord_sender.go, line 757 at r2 (raw file):

// IsTracking returns true if the heartbeat loop is running.
func (tc *TxnCoordSender) IsTracking() bool {

Can we get rid of this? If it's only used for tests then we can move the method to be test-only.

pkg/kv/txn_coord_sender.go, line 816 at r2 (raw file):

		// Abort the old txn. The client is not supposed to use use this
		// TxnCoordSender any more.
		tc.interceptorAlloc.txnHeartbeat.abortTxnAsyncLocked(ctx)

Calling closedLocked on the interceptors isn't sufficient for this?

pkg/kv/txn_coord_sender.go, line 1017 at r2 (raw file):

) {
	tc.mu.Lock()
	tc.mu.txn.Restart(pri, 0 /* upgradePriority */, ts)

Does this need to call epochBumpedLocked on the interceptors?

pkg/kv/txn_interceptor_heartbeat.go, line 67 at r2 (raw file):

	// mu contains state protected by the TxnCoordSender's mutex.
	mu struct {

nit: All of these fields are accessed under lock, not just the ones in this mu struct, so I'd flatten everything out and get rid of the mu struct to avoid confusion.

pkg/kv/txn_interceptor_heartbeat.go, line 99 at r2 (raw file):

// init initializes the txnHeartbeat. This method exists instead of a
// constructor because txnHeartbeats live in a pool in the TxnCoordSender.

A pool? No it doesn't, it's just part of a larger struct.

The method is fine though. We should probably do the same with all the other txnInterceptors.

pkg/kv/txn_interceptor_heartbeat.go, line 122 at r2 (raw file):

// closeLocked is part of the txnInteceptor interface.
func (h *txnHeartbeat) closeLocked() {

Let's keep the methods in the same order as all the other interceptors.

pkg/kv/txn_interceptor_heartbeat.go, line 140 at r2 (raw file):

	ctx context.Context, ba roachpb.BatchRequest,
) (*roachpb.BatchResponse, *roachpb.Error) {
	if h.mu.finalErr != nil {

Add a comment for this.

pkg/kv/txn_interceptor_heartbeat.go, line 188 at r2 (raw file):

		copy(ba.Requests[firstWriteIdx+1:], oldRequests[firstWriteIdx:])

		// Start the heartbeat loop. Perhaps surprisingly, this needs to be done

I'm curious about this. A batch will only need a heartbeat loop after a retryable error if it has split its EndTransaction off from the rest of its batch, right? This only happens (in DistSender) if the epoch is > 0, so do we need to do this for the first epoch? I ask because this seems very expensive for a 1PC-only workload like kv where an extremely small fraction of transactions will ever be restarted.

pkg/kv/txn_interceptor_heartbeat.go, line 207 at r2 (raw file):

	// See if we can elide an EndTxn. We can elide it for read-only transactions.
	lastIndex := int32(len(ba.Requests) - 1)
	etReq, haveEndTxn := ba.Requests[lastIndex].GetInner().(*roachpb.EndTransactionRequest)

ba.GetArg(roachpb.EndTransaction)?

pkg/kv/txn_interceptor_heartbeat.go, line 244 at r2 (raw file):

	}

	// If we inserted a begin transaction request, remove it here.

Consider pulling these steps into a series of targetted methods.

pkg/kv/txn_interceptor_sequence_nums.go, line 69 at r2 (raw file):

// populateMetaLocked is part of the txnInterceptor interface.
func (*txnSeqNumAllocator) populateMetaLocked(*roachpb.TxnCoordMeta) {}

Don't we need to populate meta.CommandCount here?

pkg/kv/txn_interceptor_sequence_nums.go, line 77 at r2 (raw file):

// epochBumpedLocked is part of the txnInterceptor interface.
func (s *txnSeqNumAllocator) epochBumpedLocked() {

Shouldn't we reset the seq num counter as well?

pkg/sql/trace_test.go, line 162 at r2 (raw file):

				"sql txn",
				"consuming rows",
				"txn coordinator send",

Either drop "send" from the txn coordinator trace or add it to the dist sender trace.

pkg/storage/batcheval/transaction.go, line 39 at r2 (raw file):

	}
	if !bytes.Equal(args.Header().Key, h.Txn.Key) {
		debug.PrintStack()

Remove.

Thanks for the review!

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/internal/client/sender.go, line 130 at r2 (raw file):

I don't know... I'd leave it...

pkg/internal/client/sender.go, line 136 at r2 (raw file):

i've collapsed the two; good idea

pkg/internal/client/sender.go, line 151 at r2 (raw file):

I've put the "write" methods before the read-only.
Happy to take suggestions.

pkg/internal/client/sender.go, line 204 at r2 (raw file):

Done.

pkg/internal/client/txn.go, line 57 at r2 (raw file):

well but the sender itself needs to be protected by a lock, unfortunately

pkg/internal/client/txn.go, line 61 at r2 (raw file):

well the txn no longer has access to the proto, and these fields are not quite write-only. I'm not sure what to do about them. Might leave them for now.

pkg/internal/client/txn.go, line 66 at r2 (raw file):

I pulled a bit and did a bit of cleanup, but I think we need to keep this field. It's useful when txn.SetUserPriority() is called a 2nd, presumably after a restart, to turn the call into a no-op instead of complaining that the txn has already been initialized.

pkg/internal/client/txn.go, line 74 at r2 (raw file):

meh. I think such a TODO would only make sense to people who already know what it's about and wouldn't help anybody else.

pkg/internal/client/txn.go, line 280 at r2 (raw file):

Done.

pkg/internal/client/txn.go, line 759 at r2 (raw file):

I don't think so... I mean, I don't think anything changed with regards to this; we test this in txn.Exec() and make the error not retriable so that a client doesn't retry.
Are you saying that we should catch it at a different layer?

pkg/internal/client/txn.go, line 819 at r2 (raw file):

well it's "replaceSenderMaybe;Locked". I prefer the "maybe" at the end (Google style), so that the method name starts with something more meaningful.

pkg/internal/client/txn_test.go, line 174 at r2 (raw file):

Done.

pkg/kv/txn_coord_sender.go, line 47 at r2 (raw file):

Done.

pkg/kv/txn_coord_sender.go, line 233 at r2 (raw file):

ok, reverted, and got rid of that assertion

pkg/kv/txn_coord_sender.go, line 462 at r2 (raw file):

Actually, I've moved the seq nums to the end, but made it skip certain requests.

pkg/kv/txn_coord_sender.go, line 496 at r2 (raw file):

Done.

pkg/kv/txn_coord_sender.go, line 670 at r2 (raw file):

but this is better!

pkg/kv/txn_coord_sender.go, line 687 at r2 (raw file):

indeed. I'll leave the TODO to motivate someone to delete all this.

pkg/kv/txn_coord_sender.go, line 757 at r2 (raw file):

Done.

pkg/kv/txn_coord_sender.go, line 816 at r2 (raw file):

it is not; closing the hb interceptor stops the hb loop, but doesn't send a rollback.
Note that calling this abortTxn() method does call closeLocked on all the interceptors.

pkg/kv/txn_coord_sender.go, line 1017 at r2 (raw file):

Done.

pkg/kv/txn_interceptor_heartbeat.go, line 67 at r2 (raw file):

well but the some of the other fields don't need to be accessed under the lock necessarily. I think it's more clear this way - separating two classes of fields.
I'm not sure where the callbacks belong; I can move those if you prefer.

pkg/kv/txn_interceptor_heartbeat.go, line 99 at r2 (raw file):

i know; it's a pool of size 1 :)
I think this wording describes the point well. I don't know how to rephrase exactly.

pkg/kv/txn_interceptor_heartbeat.go, line 122 at r2 (raw file):

Done.

pkg/kv/txn_interceptor_heartbeat.go, line 140 at r2 (raw file):

Done.

pkg/kv/txn_interceptor_heartbeat.go, line 188 at r2 (raw file):

ok, done
I've also extracted a metrics interceptor since that was tangled with the hb loop, and now it broke for 1PC txns.

pkg/kv/txn_interceptor_heartbeat.go, line 207 at r2 (raw file):

Done.

pkg/kv/txn_interceptor_heartbeat.go, line 244 at r2 (raw file):

I'm considering it, but I think it's still small enough that you're better off having everything in one place. Also cause some of the latter actions mirror some of the first ones.

pkg/kv/txn_interceptor_sequence_nums.go, line 69 at r2 (raw file):

Done.

pkg/kv/txn_interceptor_sequence_nums.go, line 77 at r2 (raw file):

Done.

pkg/sql/trace_test.go, line 162 at r2 (raw file):

Done.

pkg/storage/batcheval/transaction.go, line 39 at r2 (raw file):

Done.

r3 is #28344

Reviewable status: complete! 0 of 0 LGTMs obtained

Nice, this is looking really good. Mostly just nits at this point. I'm very happy with how readable this is becoming.

Reviewed 3 of 42 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/internal/client/sender.go, line 151 at r2 (raw file):

That looks good. Thanks.

pkg/internal/client/txn.go, line 61 at r2 (raw file):

Aren't we still duplicating state then?

pkg/internal/client/txn.go, line 759 at r2 (raw file):

Eh, I guess I'll hold my grievances for a later change.

pkg/internal/client/txn.go, line 819 at r2 (raw file):

I prefer the "maybe" at the end (Google style), so that the method name starts with something more meaningful.

Yeah but that's not what we do everywhere else. It seems very off in this codebase. I'd switch it.

pkg/kv/txn_coord_sender.go, line 670 at r2 (raw file):

It's also completely inconsistent with our codebase. consistency > personal preference.

pkg/kv/txn_coord_sender.go, line 997 at r4 (raw file):

	// Reset the state for the retry.
	tc.mu.txnState = txnPending

nit: stray line.

pkg/kv/txn_interceptor_metrics.go, line 33 at r4 (raw file):

	mu struct {
		sync.Locker // the TxnCoordSender's lock

Why do we need this lock? One of the biggest benefits of this architecture is that the interceptors don't even need to realize they're operating under lock. I don't think any of the interceptors need to have it unless they're launching async tasks. Likewise, let's collapse this struct.

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/internal/client/sender.go, line 151 at r2 (raw file):

Done.

pkg/internal/client/txn.go, line 61 at r2 (raw file):

we are duplicating state, yes. But I think it's OK enough - think of it as an implementation detail that the TCS also has this state, and it's read-only there. The Txn needs it in order to provide its API. It'd be good for this API to be different - to only allow setting these when the transaction is created and not changed afterwards.
Anyway, I wouldn't be arguing about these fields if I knew exactly what to do with them.

pkg/internal/client/txn.go, line 759 at r2 (raw file):

Done.

pkg/internal/client/txn.go, line 819 at r2 (raw file):

But consistency for consistency's sake is not a thing. Everybody who understands what "maybe" means as a prefix (not that there's much to understand) will understand it as a suffix too. And as people open their mind to the possibility of putting it at the end, it will surely take off.

pkg/kv/txn_coord_sender.go, line 670 at r2 (raw file):

litigating elsewhere

pkg/kv/txn_interceptor_metrics.go, line 33 at r4 (raw file):

yeah... Although how do you square that thought with the fact that the method is called SendLocked (which I think is a good thing)?
I've remove the lock though.

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/internal/client/txn.go, line 819 at r2 (raw file):

compromised on a new name

Reviewed 1 of 6 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/kv/txn_coord_sender.go, line 670 at r2 (raw file):

This wasn't changed.

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/kv/txn_coord_sender.go, line 670 at r2 (raw file):

changed; I was voted out in a 3-way pack first, no talking after scenario.

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/kv/txn_coord_sender.go, line 670 at r2 (raw file):

https://getyarn.io/yarn-clip/e4f43f12-95de-44b4-8dea-04a3f702e394

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/kv/txn_coord_sender.go, line 670 at r2 (raw file):

This vote wasn't a mistake.

bors r+

come on baby light my fire

Reviewable status: complete! 1 of 0 LGTMs obtained

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)