kv: decompose roles of Gossip dependency in DistSender #50116

nvanbenschoten · 2020-06-11T20:15:50Z

This change decomposes the roles of Gossip as a dependency in DistSender into that of a NodeDescStore, a source of node descriptors, and that of a FirstRangeProvider, a provider of information on the first range in a cluster.

This decomposition will be used to address #47909 by:

replacing Gossip with a TenantService as a NodeDescStore
providing a custom RangeDescriptorDB (also backed by a TenantService) instead of a FirstRangeProvider.

Together, these changes will allow us to remove DistSender's dependency on Gossip for SQL-only tenant processes.

The next step after this will be to introduce a TenantService that can satisfy these two dependencies (NodeDescStore and RangeDescriptorDB) and also use the new NodeDescStore-to-AddressResolver binding to replace the use of Gossip with the TenantService in nodedialer instances.

cockroach-teamcity · 2020-06-11T20:16:03Z

This change is

tbg

mod open discussions.

Can you give the outlook here for future releases? Do we expect to use the TenantService for all deployments once all nodes have the endpoint (i.e. 21.1)? For symmetry, I think this is preferable - we'd want DistSender to work essentially the same on tenants and KV nodes, except that the implementation of the RangeDescriptorDB on KV nodes should not incur an RPC hop, which the local server optimization will already give us.
I suspect you weren't planning to do that (or TenantServicewouldn't have been your choice of name). My expectation/hope was that the tenant-accessible RPCs are really just a subset of the KV RPCs with extra auth, validation, and more restrictions (for example, we'll have Node.Batch on both), so that we maximize coverage and converge handling of operations as much as possible, regardless of the tenancy type. Doesn't block this PR - just something to align on before your next round of PRs. If we end up doing this as I suggest, in effect you would just add these endpoints right next to (*Node).Batch and they would get mirrored into a service TenantKV in my follow-up to #50082.

PS: I would even apply the same argument to NodeDescStorage and any future endpoint really - serve them from the same endpoint the tenants would use (but on the KV service) so that it all works the same for tenants and integrated nodes alike).

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 3 of 3 files at r4, 15 of 15 files at r5.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

pkg/gossip/gossip.go, line 985 at r4 (raw file):

// getNodeIDAddressLocked looks up the SQL address of the node by ID. The mutex
// is assumed held by the caller. This method is called externally via
// GetNodeIDSQLAddress.

This sentence is pretty much removable, right? A method XLocked ~always gets called by X.

pkg/kv/kvclient/kvcoord/dist_sender.go, line 266 at r1 (raw file):

		gossip:     g,
		metrics:    makeDistSenderMetrics(),
		nodeDialer: cfg.NodeDialer,

The way I like to rewrite these methods is by initializing everything needed to make a &DistSender before making the &DistSender (so that we never deal in a half-inited struct). Might be better done separately though.

pkg/kv/kvclient/kvcoord/dist_sender.go, line 260 at r4 (raw file):

// DistSenderContext or the fields within is optional. For omitted values, sane
// defaults will be used.
func NewDistSender(cfg DistSenderConfig, g *gossip.Gossip) *DistSender {

❤️

pkg/kv/kvclient/kvcoord/dist_sender.go, line 189 at r5 (raw file):

	// the first range has changed. The callback is passed the RangeDescriptor
	// in its marshaled form.
	OnFirstRangeChanged(func(roachpb.Value))

Wouldn't it be more idiomatic to pass the *RangeDescriptor here? I think you're trying to avoid that struct leaking into Gossip, but IMO it would be about as idiomatic (perhaps more) to have an adapter in this package that wraps a *Gossip into a FirstRangeProvider anyway, so that gossip wouldn't have to know about FirstRangeProvider.

pkg/kv/kvclient/kvcoord/dist_sender.go, line 209 at r5 (raw file):

	// which span ranges and don't require read consistency.
	clock *hlc.Clock
	// nodeDescs provides information on other nodes that DistSender may

why "other nodes"? DistSender will also run on tenants, so we could be more explicit and say "KV nodes":

// nodeDescs provides information on the KV nodes that DistSender may consider routing requests to.

pkg/kv/kvclient/kvcoord/dist_sender.go, line 217 at r5 (raw file):

	rangeCache *RangeDescriptorCache
	// firstRangeProvider provides the range descriptor for range one.
	// The dependency is optional.

What does this sentence mean? If you don't give one then... what? I'm seeing the answer further below, but maybe hint at it by saying "This is not required if a range descriptor db is supplied".

pkg/kv/kvclient/kvcoord/dist_sender.go, line 259 at r5 (raw file):

	NodeDialer      *nodedialer.Dialer

	// One of the following dependencies must be provided, but not both.

Could this be explained more easily by saying:

One of the following two must be provided, but not both.

If FirstRangeProvider is supplied, DistSender will use itself as a RangeDescriptorDB and scan the meta ranges directly (using the FirstRangeProvider to bootstrap the location of the meta1 range). Additionally, it will proactively update its range descriptor cache with any meta1 updates from the provider.

If RangeDescriptorDB is provided, all range lookups will be delegated to it.

Is the intention that we drop the Gossip dependency when the migration is through? If so, can we stop listening to meta1 updates in the DistSender itself today already and really just hand the FirstRangeProvider through to the RangeDescriptorDB (which is right now implemented by DistSender itself, but a small layer between wouldn't hurt)?
That way we will have more confidence that we're not relying on the meta1 updates going into the range desc cache proactively (it should not matter that it does other than that this enables DistSender's use as a RangeDescriptorDB)? I think I would prefer that.

For simplicity, I would also then say that you provide exactly one but are prevented from supplying both. (You have conflicting statements about that in the comment right now).

I'm not sure why we were doing this.

A few of the comments were stale, primarily around the structured value that was associated with a given key.

Removes a context.TODO().

While doing this, remove a pessimization. Not only was the localityTierMap not worth it given how few locality tiers we expect in practice, but it was represented as a `map[string]struct{}` instead of a `map[roachpb.Tier]struct{}`, meaning that we needed to stringify each LocalityAddress tier before each map access, which forced memory allocations and of extra work. An n^2 approach is simpler and almost certainly faster up to ~100 locality tiers.

nvanbenschoten

Yes, you're right that I wasn't planning on merging the tenant service into the existing Internal service, but that was mostly driven by following the path of least resistance. I agree with your stance on all of this and favor maintaining as much symmetry between DistSender on tenant and KV nodes.

The one wrinkle here is that the TenantService client that I've been prototyping maintains some state that only makes sense in the tenant configuration. For instance, it implements NodeDescStorage off of a local cache that is asynchronously updated from a gRPC stream on a new NodeInfo RPC. We don't need any of this caching on KV nodes, so we wouldn't want them to have to pay for it.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @tbg)

pkg/gossip/gossip.go, line 985 at r4 (raw file):