cockroachdb / cockroach

CockroachDB — the cloud native, distributed SQL database designed for high availability, effortless scale, and control over data placement.
https://www.cockroachlabs.com
Other
29.97k stars 3.79k forks source link

kvserver: use cached store capacity in more calls #94929

Open kvoli opened 1 year ago

kvoli commented 1 year ago

Is your feature request related to a problem? Please describe.

store.Capacity() is expensive as it iterates over every replica. It is called periodically on the same interval by different tickers, however we don't use the cached value - meaning that we wastefully recompute it from scratch despite it not being very stale.

This wastes CPU resources, especially in a high replica count cluster.

We calculate the store capacity periodically from these places:

Describe the solution you'd like

Use Capacity(true /* useCached */) more regularly, rather than have each ticking component which is calling at the same interval calculate from scratch. Ideally, this useCached would switch to a staleness interval the caller is willing to accept the cached value from before recalculating.

Jira issue: CRDB-23231

petermattis commented 9 months ago

Do all of these code paths need an up-to-date StoreCapacity? I can imagine getting rid of the useCached argument to Capacity() and internally tracking the last time capacity was updated and only refresh it once every 10s. Looks like the biggest headache might be various code paths from test code. Alternately, rather than having 3 separate background loops, could we have one loop which recomputes StoreCapacity and then invokes metrics recomputation, writing of the node status, and gossiping of the store?

kvoli commented 9 months ago

Do all of these code paths need an up-to-date StoreCapacity?

All of these code paths do not need an up to date StoreCapacity. The StoreCapacity will be gossiped before the 10s period if the capacity changes significantly. The store metrics would have a larger potential gap between nodes in a cluster for the same tick in TSDB IIUC, however that doesn't appear problematic.

[...] can imagine getting rid of the useCached argument to Capacity() and internally tracking the last time capacity was updated and only refresh it once every 10s.

This would be a nice solution. It would be ideal if we could consolidate the three background loops together, given they are operating on the same interval anyway. I'd expect the loop start times to be similar, so perhaps the first suggestion would be simpler and to similar effect as the second anyway.