kvserver: record batch requests with no gateway #85178

kvoli · 2022-07-28T00:05:51Z

Previously, batch requests with no GatewayNodeID would not be
accounted for on the QPS of a replica. By extension, the store QPS would
also not aggregate this missing QPS over replicas it holds. This patch
introduces tracking for all requests, regardless of the GatewayNodeID.

This was done to as follow the workload lease transfers consider the
per-locality counts, therefore untagged localities were not useful. This
has since been updated to ignore filter out localities directly, so it
is not necessary to exclude them anymore.

leaseholderStats, which previously tracked the QPS, and writeStats
tracking the mvcc keys written, have also been removed. They are
duplicated in batchRequest and writeKeys respectively, within the
loadStats of a replica.

resolves #85157

Release note: None

cockroach-teamcity · 2022-07-28T00:05:59Z

This change is

lidorcarmel

Reviewed 18 of 18 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @kvoli)

pkg/kv/kvserver/replica_application_state_machine.go line 1014 at r1 (raw file):

	// Record the write activity, passing a 0 nodeID because replica.writeStats
	// intentionally doesn't track the origin of the writes.
	b.r.loadStats.writeKeys.RecordCount(float64(b.mutations), 0)

is it guaranteed that loadStats cannot be nil here?

Code quote:

b.r.loadStats.writeKeys.RecordCount(float64(b.mutations), 0

pkg/kv/kvserver/replica_send.go line 1006 at r1 (raw file):

func (r *Replica) recordBatchRequestLoad(ctx context.Context, ba *roachpb.BatchRequest) {
	if r.loadStats == nil {
		log.Eventf(

when would this happen? only in tests maybe? if in prod, will it spam the log? if yes then either move this to be a level N log (vmodule) or log every N messages or something like that..

Code quote:

r.loadStats == nil {

pkg/kv/kvserver/replica_send.go line 1017 at r1 (raw file):

	// estimate of a BatchRequest. See getBatchRequestQPS() for the
	// calculation.
	scaledQPS := r.getBatchRequestQPS(ctx, ba)

nit: adjustedQPS maybe? when I see scale I think of other things, sorry :(

Code quote:

scaledQPS

pkg/kv/kvserver/replica_send.go line 1023 at r1 (raw file):

	// requests, for follow the workload lease transfers. We now record these
	// regardless of the gateway node id, as the only user of this information
	// does not consider untagged localities. See

I'm wondering whether this really has value here? definitely this should be in the PR/commit description, and it is, but do readers a year from now would need to know this info? feel free to keep it if you think it has any value..

Code quote:

	// NB: Previously, we would ignore batch requests which had no gateway node
	// id. This was in order to retrieve accurate per-locality counts of
	// requests, for follow the workload lease transfers. We now record these
	// regardless of the gateway node id, as the only user of this information
	// does not consider untagged localities. See
	// allocator.shouldTransferLeaseForAccessLocality() .

pkg/kv/kvserver/store_merge.go line 149 at r1 (raw file):

	if leftRepl.leaseholderStats != nil {
		leftRepl.leaseholderStats.ResetRequestCounts()

question: what does this change mean? that we used to reset qps stats here and now we don't how will it affect the behavior after merge? I'll try to look later.

Code quote:

	if leftRepl.leaseholderStats != nil {
		leftRepl.leaseholderStats.ResetRequestCounts()
	}
	if leftRepl.writeStats != nil {
		// Note: this could be drastically improved by adding a replicaStats method
		// that merges stats. Resetting stats is typically bad for the rebalancing
		// logic that depends on them.
		leftRepl.writeStats.ResetRequestCounts()
	}

kvoli

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

pkg/kv/kvserver/replica_application_state_machine.go line 1014 at r1 (raw file):