kv: consolidate RangeInfos in RPC responses by andreimatei · Pull Request #51168 · cockroachdb/cockroach

andreimatei · 2020-07-08T18:37:24Z

The kvclient can request information from the server about the
descriptor and lease of the range evaluating a request. This is
requested by setting a bit on the batch's header. The server was then
replying with information in every single request's header (as opposed
to the BatchResponse's header). For all the requests that hadn't been
split by the DistSender, the responses were the same.

This patch moves the responses to the BatchResponse's header - so, for
one RPC, there's gonna be exactly one response although the DistSender
will combine multiple of them. For backwards compatibility with 20.1,
new servers still populate the old location, and new clients copy from
the old location to the new one (subject to a cluster version gate).

The patch also removes the latching of a range's descriptor and lease
keys when requests will return range information. The latching was not
necessary - the respective keys are not actually read from the database
(in-memory caches are used) and clients don't care about any particular
consistency guarantees. In order to do away with this latching cleanly,
the server-side code that populates the range info is lifted from the
lower levels of request evaluation to a high-level.

This patch is motivated by upcoming changes which will make returning
range info more common. As such, both the latching and the duplication
of returned data become problems.

Release note: None

cockroach-teamcity · 2020-07-08T20:21:49Z

This change is

nvb

I like where this is going.

Reviewed 2 of 2 files at r1, 14 of 14 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei)

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

				len(reply.RangeInfos) == 0 &&
				!ds.st.Version.IsActive(ctx, clusterversion.VersionClientRangeInfosOnBatchResponse) {
				if ba.ReturnRangeInfo && len(reply.RangeInfos) == 0 {

Are we checking this condition twice?

pkg/kv/kvserver/replica_send.go, line 123 at r2 (raw file):

	// and a pErr here. Also, some errors (e.g. NotLeaseholderError) have custom
	// ways of returning range info.
	if ba.ReturnRangeInfo && pErr == nil {

Let's move this into a helper function to avoid cluttering this method.

pkg/kv/kvserver/replica_send.go, line 125 at r2 (raw file):

	if ba.ReturnRangeInfo && pErr == nil {
		lease, _ := r.GetLease()
		desc := r.Desc()

Didn't you just add an accessor to atomically grab both of these?

pkg/kv/kvserver/replica_send.go, line 139 at r2 (raw file):

				reply := r.GetInner()
				header := reply.Header()
				header.DeprecatedRangeInfos = []roachpb.RangeInfo{

nit: header.DeprecatedRangeInfos = br.RangeInfos

pkg/roachpb/api.go, line 465 at r2 (raw file):

			continue
		}
		h.RangeInfos = append(h.RangeInfos[:i], append([]RangeInfo{ri}, h.RangeInfos[i:]...)...)

This will result in wasted allocations. How abound something like:

h.RangeInfos = append(h.RangeInfos, roachpb.RangeInfo{})
copy(h.RangeInfos[i+1:], h.RangeInfos[i:])
h.RangeInfos[i] = ri

andreimatei

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

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