roachtest: add benchmark for INSPECT under admission control #155144
Conversation
|
This change is |
spilchen
force-pushed
the
gh-154457/251009/0819/inspect-overload-workload/pr-ready
branch
2 times, most recently
from
85566a6 to
b4325f0
Compare
spilchen
force-pushed
the
gh-154457/251009/0819/inspect-overload-workload/pr-ready
branch
2 times, most recently
from
0f0344e to
9e57996
Compare
There was a problem hiding this comment.
really nice tests! i just had some small questions
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @spilchen)
pkg/cmd/roachtest/tests/admission_control_inspect.go line 45 at r1 (raw file):
return registry.TestSpec{ Name: fmt.Sprintf("inspect/admission-control/rows=%d", numRows),
nit: it may be useful to include the cpu count and node count in the name. for example, see the naming convention of other test names at https://roachperf.crdb.dev/ (e.g. cdc/scan/catchup-cold/nodes=5/cpu=16/rows=1G/ranges=100/protocol=mux/format=json/sink=null). it's helpful since we also need code in roachperf to display these charts, and the charting logic is based on the test name.
pkg/cmd/roachtest/tests/admission_control_inspect.go line 232 at r2 (raw file):
t.L().Printf("Setting workload duration to %v (based on calibration: %v)\n", workloadDuration, calibrationDuration) // Start read-only foreground workload using kv (since bulkingest doesn't support reads).
is there a specific reason that we want a read only workload here? what would the issue be with having a workload with writes? perhaps the answer could be included in this code comment.
pkg/cmd/roachtest/tests/admission_control_inspect.go line 248 at r2 (raw file):
time.Sleep(baselineDuration) _ = runInspect(
nit: for these calls to runInspect, it could be useful to keep the return value and log the duration. seeing the durtaion in logs might be useful when manually looking at logs.
Add a new roachtest to measure the impact of admission control on INSPECT operations running concurrently with foreground workloads. As part of this, introduce a cluster setting knob to control admission control behavior for INSPECT operations, allowing them to be run with either BulkLowQoS (enabled) or Normal QoS (disabled). The test performs three INSPECT runs: 1. A calibration run (with no workload) to dynamically determine test duration. 2. An INSPECT run with admission control enabled (BulkLowQoS) during a foreground read workload. 3. An INSPECT run with admission control disabled (Normal QoS) during the same workload. Each run measures and reports throughput (rows/s/CPU), enabling us to quantify: - The performance impact of admission control on INSPECT operations. - The effectiveness of admission control in protecting foreground workload latency. The workload duration is dynamically calculated to ensure it's long enough to encompass both INSPECT runs. Informs: cockroachdb#154457 Epic: CRDB-30356 Release note: none
spilchen
force-pushed
the
gh-154457/251009/0819/inspect-overload-workload/pr-ready
branch
from
9e57996 to
6aa1028
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/roachtest/tests/admission_control_inspect.go line 45 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: it may be useful to include the cpu count and node count in the name. for example, see the naming convention of other test names at https://roachperf.crdb.dev/ (e.g.
cdc/scan/catchup-cold/nodes=5/cpu=16/rows=1G/ranges=100/protocol=mux/format=json/sink=null). it's helpful since we also need code in roachperf to display these charts, and the charting logic is based on the test name.
I'll use this naming convention for the other inspect roachtest too.
pkg/cmd/roachtest/tests/admission_control_inspect.go line 232 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
is there a specific reason that we want a read only workload here? what would the issue be with having a workload with writes? perhaps the answer could be included in this code comment.
INSPECT is read-only, so I would expect contention with another workload to be only on reads too. Also, I wanted a static dataset since we run INSPECT multiple times. I wanted it doing the same amount of work each time, just under different load characteristics.
pkg/cmd/roachtest/tests/admission_control_inspect.go line 248 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: for these calls to runInspect, it could be useful to keep the return value and log the duration. seeing the durtaion in logs might be useful when manually looking at logs.
The runInspect captures that with this log message:
https://github.com/cockroachdb/cockroach/pull/155144/files#diff-094b401c27f4a7015bb40ca9c4c9f24b3d9708948251fc5a5b796e5c10d061b6R175-R179
|
TFTR! bors r+ |
3 participants
Add a new roachtest to measure the impact of admission control on INSPECT operations running concurrently with foreground workloads.
As part of this, introduce a cluster setting knob to control admission control behavior for INSPECT operations, allowing them to be run with either BulkLowQoS (enabled) or Normal QoS (disabled).
The test performs three INSPECT runs:
Each run measures and reports throughput (rows/s/CPU), enabling us to quantify:
The workload duration is dynamically calculated to ensure it's long enough to encompass both INSPECT runs.
Informs: #154457
Epic: CRDB-30356
Release note: none