kvserver: improve the accounting of AddSSTableRequests' QPS

[aayushshah15]

Currently, our QPS accounting is a little broken because we only record the count of incoming BatchRequests into a replica (not the actual count of requests in those batches). One of the consequences of this is that AddSSTTableRequests only count as 1 QPS even though their actual load on the receiving hardware is orders of magnitude higher than that. This means that during an import, without any background traffic in the cluster, CRDB's StoreRebalancer (which is responsible for QPS based lease and replica rebalancing) doesn't even kick in (because it doesn't try resolving tiny QPS differences between stores). We've seen at least one support case where this all lead to severe load imbalance among the nodes in a customer's cluster.

One idea by @nvanbenschoten was to consider AddSSTableRequests to be something like 1000 requests when recording them in the leaseholder replica's leaseholderStats.

This is likely a small change but we'd want to run a few experiments to validate it (same as #50620).

cc. @cockroachdb/kv-notifications

[erikgrinaker]

Following #72085, once all callers are migrated to use WriteAtRequestTimestamp, we'll know during evaluation how many keys the SST contains, and could propagate this back into the stats. As a fallback, we may want a linear cost based on the SST byte size, since they can vary quite a bit.

[nvanbenschoten]

@dt mentioned that he landed something related in #42277. Unfortunately, that PR updated the writeStats on each replica, but only the leaseholderStats is used to make load-based rebalancing decisions, so we wouldn't have expected that change to help here.

[tbg]

One idea by @nvanbenschoten was to consider AddSSTableRequests to be something like 1000 requests when recording them in the leaseholder replica's leaseholderStats.

Can't AddSSTableRequests contain the number of keys the SST includes (and perhaps the total length of the keys and/or bytes in case we want to track something based on that later)? That doesn't seem difficult to achieve and then we can properly solve the problem.

[erikgrinaker]

Can't AddSSTableRequests contain the number of keys the SST includes (and perhaps the total length of the keys and/or bytes in case we want to track something based on that later)? That doesn't seem difficult to achieve and then we can properly solve the problem.

Actually, I think they already do most of the time:
https://github.com/cockroachdb/cockroach/blob/0c8383cbc91da37846584e08eff5060e0e14cdfa/pkg/roachpb/api.proto#L1681-L1685

Notably, this is always provided during bulk ingestion:

cockroach/pkg/kv/bulk/sst_batcher.go

Lines 470 to 472 in 2318073

	err = db.AddSSTable(ctx, item.start, item.end, item.sstBytes, false, /* disallowConflicts */
	!item.disallowShadowingBelow.IsEmpty(), item.disallowShadowingBelow, &item.stats,
	ingestAsWriteBatch, batchTs, false /* writeAtBatchTs */)

[erikgrinaker]

AddSSTTableRequests only count as 1 QPS

I was under the impression that we recorded reads (QPS) and writes (WPS) separately. AddSSTable requests are already accounted for in WPS:

cockroach/pkg/kv/kvserver/replica_application_state_machine.go

Line 645 in 36f61dc

b.r.writeStats.recordCount(float64(added), 0)

The proposal here seems to be to better include AddSSTable writes in QPS. Wouldn't it be better to change the store rebalancer to take WPS into account for rebalancing decisions too, separately from QPS, instead of including all writes in QPS? Nodes can generally handle much larger amounts of reads than writes, and so even if a replica is seeing a high write volume, it may not get split/rebalanced because it's drowned out by an even higher read volume -- so it seems to make sense to keep these signals separate.

[erikgrinaker]

I did a proof of concept of the initial proposal in https://github.com/erikgrinaker/cockroach/tree/sst-qps, where both the number of requests in a batch and also the number of keys in an SST count towards QPS. Note that index backfills do not include MVCC stats by default because they do not hit this branch:

cockroach/pkg/kv/bulk/sst_batcher.go

Lines 208 to 210 in 2cce39c

	if !b.disallowShadowingBelow.IsEmpty() {
	b.updateMVCCStats(key, value)
	}

This was a marked improvement, where load did get distributed across other nodes as well, although this took a fairly long time to converge (>30 minutes), and even after that the original three nodes were still running about twice as hot than the others. See this internal Slack thread for results and graphs. @aayushshah15 points out that this may also be due to fallout from #65379.

Either way, this approach seems viable, but will need additional testing and tuning. For posterity, this was tested as follows:

export GCE_PROJECT=cockroach-ephemeral
cluster=grinaker-index
roachprod create ${cluster} --nodes=10 --gce-machine-type=n2-standard-4 --gce-zones=europe-west1-c --local-ssd=false --gce-pd-volume-size=500 --lifetime=168h
roachprod stage ${cluster} cockroach
roachprod start ${cluster} --racks=10

CREATE TABLE trade (t_id INT8 NOT NULL PRIMARY KEY, t_dts TIMESTAMP NOT NULL, t_st_id VARCHAR(4) NOT NULL, t_tt_id VARCHAR(3) NOT NULL, t_is_cash BOOL NOT NULL, t_s_symb VARCHAR(15) NOT NULL, t_qty INT4 NOT NULL CHECK (t_qty > 0), t_bid_price DECIMAL(8,2) NOT NULL CHECK (t_bid_price > 0), t_ca_id INT8 NOT NULL, t_exec_name VARCHAR(49) NOT NULL, t_trade_price DECIMAL(8,2), t_chrg DECIMAL(10,2) NOT NULL CHECK (t_chrg >= 0), t_comm DECIMAL(10,2) NOT NULL CHECK (t_comm >= 0), t_tax DECIMAL(10,2) NOT NULL CHECK (t_tax  >= 0), t_lifo BOOL NOT NULL, FAMILY (t_id), FAMILY (t_comm, t_dts, t_st_id, t_trade_price, t_exec_name, t_tt_id, t_is_cash, t_s_symb, t_qty, t_bid_price, t_ca_id, t_chrg, t_lifo), FAMILY (t_tax));
IMPORT INTO trade CSV DATA ('gs://cockroach-fixtures/tpce-csv/customers=500000/*/Trade.txt?AUTH=implicit') WITH delimiter = '|', DETACHED;
CREATE INDEX ON trade (t_ca_id, t_dts DESC) STORING (t_st_id, t_tt_id, t_is_cash, t_s_symb, t_qty, t_bid_price, t_exec_name, t_trade_price, t_chrg);