-
Notifications
You must be signed in to change notification settings - Fork 4k
asim: specify node capacity in cores, not nanos/sec #158144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
8972085 to
b418f28
Compare
|
For now, i only updated the the node capacity in the input format, but not in the underlying data structures, like BasicCluster.NodeCPURateCapacity and ClusterInfo.NodeCPURateCapacityNanos. Should the CPU units be updated there too, or is it only the input format we're worried about? |
wenyihu6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think only the input, so this should be enough.
Reviewable status:
complete! 1 of 0 LGTMs obtained
pkg/kv/kvserver/asim/state/node_cpu_cores_test.go line 14 at r1 (raw file):
) func TestNodeCPUCores_ToRateCapacityNanos(t *testing.T) {
Nice test!
tbg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
@tbg reviewed 15 of 15 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @angeladietz)
pkg/kv/kvserver/asim/config/settings.go line 36 at r1 (raw file):
const ( DefaultNodeCPUCores = 8.0 // 8 vcpus DefaultNodeCPURateCapacityNanos = 8 * 1e9 // 8 vcpus
nit: make this DefaultNodeCPUCores * 1e9.
Previously, the datadriven asim tests expressed node capacity in nanoseconds/core which are difficult to read due to lots of zeros. Now, the node capacity is expressed in cores (float) which is much more readable and less prone to user error. For example, `node_cpu_rate_capacity=8000000000` is now expressed as `node_cpu_cores=8`. Fixes cockroachdb#156845 Epic: CRDB-55052 Release note: none.
b418f28 to
8d22968
Compare
|
TFTRs! bors r+ |
|
bors r+ |
ah good to know, thanks! |
Previously, the datadriven asim tests expressed node capacity in nanoseconds/core which are difficult to read due to lots of zeros. Now, the node capacity is expressed in cores (float) which is much more readable and less prone to user error. For example,
node_cpu_rate_capacity=8000000000is now expressed asnode_cpu_cores=8.Fixes #156845
Epic: CRDB-55052
Release note: none.