-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
workload/tpcc: extend --wait to also support fractions #39685
Conversation
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'm okay with this, though we should probably deprecate the --waits flag since --wait-fraction=0 should be the same thing, right? Mind running this by nathan or awerner to see if they have thoughts on whether tpcc is the right thing for what you'd looking for?
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz and @tbg)
I have talked to @nvanbenschoten and we agreed that this is an OK way to go forward, as long as the users of the flag are mindful that it changes the amount of contention. @danhhz what is it that you'd like me to do about |
You could change the existing |
I was imagining adding a deprecation notice to the --waits flag description. If that makes 19.2, I'd be comfortable removing the flag in 20.1 |
I understand how to do this. I think it is a good idea because it's upward compatible and doesn't require a deprecation cycle. I'll try that. |
74c2b77
to
1ea411b
Compare
Modified as suggested, RFAL |
1ea411b
to
3ce10d4
Compare
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.
this is great, much better than what I was suggesting.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz and @tbg)
pkg/workload/tpcc/tpcc.go, line 157 at r1 (raw file):
`split`: {RuntimeOnly: true}, `wait`: {RuntimeOnly: true}, `wait-fraction`: {RuntimeOnly: true},
no longer necessary, right?
3ce10d4
to
8cbe440
Compare
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @danhhz and @tbg)
pkg/workload/tpcc/tpcc.go, line 157 at r1 (raw file):
Previously, danhhz (Daniel Harrison) wrote…
no longer necessary, right?
Indeed, fixed.
bors r=danhhz,petermattis |
In order to use TPC-C for operational testing (i.e. NOT benchmarking) it is useful to increase the rate of transactions without altogether saturating the TPS capacity (`--wait=false`). For this purpose, this commit extends the syntax accepted by `--wait` to recognize a multiplier which is applied to all wait times. The default is 1.0 (i.e. 100% of the wait time required by a benchmark). The words `true`/`on` and `false`/`off` are aliases for 1.0 and 0.0, respectively. Release note: None
8cbe440
to
c6ec74f
Compare
Canceled |
there was a lint failure, sorry about that bors r=danhhz,petermattis |
39685: workload/tpcc: extend --wait to also support fractions r=danhhz,petermattis a=knz In order to use TPC-C for operational testing (i.e. NOT benchmarking) it is useful to increase the rate of transactions without altogether saturating the TPS capacity (`--wait=false`). For this purpose, this commit extends the syntax accepted by `--wait` to recognize a multiplier which is applied to all wait times. The default is 1.0 (i.e. 100% of the wait time required by a benchmark). The words `true`/`on` and `false`/`off` are aliases for 1.0 and 0.0, respectively 40380: exec: use same decimal context as non-vec r=rafiss a=rafiss Use the same decimal contexts that eval.go uses for decimal arithmetic. based on discussion in #40327 Release note: None Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com> Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Build succeeded |
@@ -200,15 +241,15 @@ func (w *tpcc) Hooks() workload.Hooks { | |||
// waiting, we only use up to a set number of connections per warehouse. | |||
// This isn't mandated by the spec, but opening a connection per worker | |||
// when they each spend most of their time waiting is wasteful. | |||
if !w.doWaits { | |||
if w.waitFraction == 0 { | |||
w.numConns = w.workers | |||
} else { | |||
w.numConns = w.activeWarehouses * numConnsPerWarehouse |
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.
Do we need to scale the number of connections by the wait fraction? I'd expect them to saturate as-is with a fraction like 0.1
.
In order to use TPC-C for operational testing (i.e. NOT benchmarking)
it is useful to increase the rate of transactions without
altogether saturating the TPS capacity (
--wait=false
).For this purpose, this commit extends the syntax accepted by
--wait
to recognize a multiplier which is applied to all wait times. The
default is 1.0 (i.e. 100% of the wait time required by a benchmark).
The words
true
/on
andfalse
/off
are aliases for 1.0 and 0.0,respectively