This change is 

General comment: the lack of wrapping here makes it pretty hard to read in e.g. reviewable.

Looks good to me, though I'm not sure what the point of this RFC is - this doesn't seem like a contentious issue, and the RFC is mostly laying out the problem rather than proposing a specific solution.

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

docs/RFCS/client.Txn-TxnCoordSender_split.md, line 65 at r1 (raw file):

1. What do we do about the present cases where the *client* is actually not collocated with the `TxnCoordSender`? Namely, there is an `ExternalClient` RPC interface that can be used to connect a `client.Txn` to a remote `TxnCoordSender` via RPCs. This used by some debug CLI commands and by implementers of the [Cluster](https://github.com/cockroachdb/cockroach/blob/f7315c0/pkg/acceptance/cluster/cluster.go#L36) interface (used in some acceptance tests). All these uses seem to be vestiges of another age and another mindset.

I don't understand why this is a problem - this client will become its own transaction coordinator. Earlier in the text you mention that we didn't do this in the KV era because we'd have to maintain many implementations (one per language) but we already have the Go implementation (which will subsume the coordinator), and it seems OK to use it, even outside of Cockroach, if we decide to permit it (though I'm not advocating for that here, of course).

Comments from Reviewable

wrapped everything.

The point is that @bdarnell in the past showed reluctance to uniting these two layers. This is to cover my ass.

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

Comments from Reviewable

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

Comments from Reviewable

I'm in favor generally, though I agree with @tamird that the actual proposal is light on details. Perhaps you could make this more concrete:

• Step1: wholesale pull TxnCoordSender into client.Txn. Is this feasible? Maybe allocates a lot and the heartbeat goroutine per client.Txn may not be great. We should provide details on what's the goal here.
• Step2: absorb TxnCoordSender into client.Txn. What will the new client.Txn look like?

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

docs/RFCS/client.Txn-TxnCoordSender_split.md, line 65 at r1 (raw file):

Yeah, I also think that case would be possible but we should just remove these hold-overs.

docs/RFCS/client.Txn-TxnCoordSender_split.md, line 6 at r2 (raw file):

- Authors: Andrei Matei (andreimatei1@gmail.com)
- RFC PR: 16000
- Cockroach Issue: 13376, 10511

Mind adding # here?

Comments from Reviewable

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

docs/RFCS/client.Txn-TxnCoordSender_split.md, line 65 at r1 (raw file):

What would you replace them with? I believe they're present for cases where we don't have SQL commands for certain operations.

Comments from Reviewable

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

docs/RFCS/client.Txn-TxnCoordSender_split.md, line 65 at r1 (raw file):

Worth making a list of code affected by this, but anything that runs a few simple commands can be done server-side via a new endpoint, and code that acts like a real external client perhaps should just go away.

Comments from Reviewable

I've added a few absorption steps. But... I reserve the right to be compiler-driven at times.

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

docs/RFCS/client.Txn-TxnCoordSender_split.md, line 65 at r1 (raw file):

I'm less convinced that having a "coordinator" outside of the cluster is really feasible. For one, you need to be able to assign timestamps according to a cluster's clock. You also need to interface with a node's metrics to record state transitions.

docs/RFCS/client.Txn-TxnCoordSender_split.md, line 6 at r2 (raw file):

Done. I wondered about that... Will it do anything?

docs/RFCS/client.Txn-TxnCoordSender_split.md, line 18 at r2 (raw file):

Well I do understand the components... The thesis is that the components are in our way; they're giving us headaches and no joy. I don't know what else to do than merge as much as possible. But I've listed a few steps at the end, maybe that helps.

Comments from Reviewable

While this is definitely a bit different from a standard RFC proposal, I appreciate having the reasoning for the change clearly documented. Thanks @andreimatei

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

docs/RFCS/client.Txn-TxnCoordSender_split.md, line 6 at r2 (raw file):

Nope, it doesn't do anything. If you want it to be a link you have to manually add the link via markdown

docs/RFCS/client.Txn-TxnCoordSender_split.md, line 2 at r3 (raw file):

- Feature Name: Eliminating the `client.Txn` ↔ `TxnCoordSender` split
- Status: draft/in-progress/completed/rejected/obsolete

draft? in-progress?

docs/RFCS/client.Txn-TxnCoordSender_split.md, line 26 at r3 (raw file):

KV requests go through a hierarchy of `Sender`s, on their way from a client
(i.e. code in the `internal/client` package, notably `client.DB` (a handle to a

https://xkcd.com/859/

Comments from Reviewable

I'm not opposed to this move in principle, I'm just concerned about the cost vs benefit. There's going to be a lot of test cleanup work to get everything fully moved over. I felt like the cleanup work wasn't worth it pre-distsql. Maybe distsql's issues with this interface are reason enough to make the move, but I'm not fully convinced. Is there a useful middle ground?

For example, maybe we make client.DB contain a *kv.TxnCoordSender instead of a client.Sender and add new methods to TxnCoordSender. Then client.Txn could start using a richer interface to communicate with the sender without disrupting too much else (we'd still need to get rid of ExternalClient, but I think this would let us avoid a bunch of changes). We could move functionality from one to the other incrementally as it makes sense (putting the heartbeat loop on client.Txn seems like a good idea, for example. The tests that use TxnCoordSender without a client.Txn generally don't run long enough for heartbeating to matter)

Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

docs/RFCS/client.Txn-TxnCoordSender_split.md, line 65 at r1 (raw file):

Yeah, we should rewrite the acceptance tests to use SQL instead of ExternalClient and expose whatever the debugging CLI commands need through either new RPCs or SQL virtual tables. Nothing external to the process should be doing KV transactions.

There are also a lot of tests (citation needed) that use transactions via the TxnCoordSender without a client.Txn object. These are in process so there aren't the same issues as ExternalClient, but I think it's still a lot of work to clean it up.

docs/RFCS/client.Txn-TxnCoordSender_split.md, line 77 at r2 (raw file):

   to work for the SQL layer’s use of the `TxnCoordSender`, as all queries in a
   SQL transaction share a context, but we’ll probably need separate contexts in
   the future when we’ll want to support cancelling individual queries.

I'm not sure that we'll ever want to support cancelling individual queries; I think cancellation can always work at the transaction (or session) level. But I agree that it would be cleaner to do the heartbeat at a level that is naturally scoped to the life of the transaction instead of abusing the context passed on the first request.

docs/RFCS/client.Txn-TxnCoordSender_split.md, line 106 at r2 (raw file):

The DistSQL issues stem from the fact that KV requests performed by DistSQL
queries don’t go through the `TxnCoordSender` on the gateway (they also don’t go
through `TxnCoordSenders` on other nodes, but that’s less relevant to this

But the operations that don't go through TxnCoordSender don't go through client.Txn either, right? So how does merging the two address these issues? Is it just that client.Txn can have a richer interface and let us access its state separately from sending requests through it?

Comments from Reviewable

For example, maybe we make client.DB contain a *kv.TxnCoordSender instead of a client.Sender and add new methods to TxnCoordSender

I like the idea of doing the logical move before the physical one.

Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

Comments from Reviewable

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

Comments from Reviewable

maybe we make client.DB contain a *kv.TxnCoordSender instead of a client.Sender

Not sure if this is a good idea for production, but alternative impls of this sender is how a bunch of the "read only queries over backups" and "bulk load" prototypes I've built have worked. I'd hate to lose it

Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

Comments from Reviewable

reminder: don't forget to rebase if you plan to continue work on this, and pick up the new RFC sections + naming convention.

the work is being done in #28185