Skip to content
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: check the 90th percentile instead of the .9th #72285

Merged
merged 1 commit into from
Nov 1, 2021

Conversation

ajwerner
Copy link
Contributor

@ajwerner ajwerner commented Nov 1, 2021

The hdrhistogram API was misleading. It expected a percentile for a quantile
calculation. All-in-all it appears the author of that library had percentile
and quantile confused. At some point in the last year, somebody did add a
ValueAtPercentile method which has the identical behavior as
ValueAtQuantile.

See https://github.com/cockroachdb/vendored/blob/5b815c7d468d59337db2c742373183af5525e729/github.com/codahale/hdrhistogram/hdr.go#L241-L259

Fixes #72282.

Release note: None

The hdrhistogram API was misleading. It expected a percentile for a quantile
calculation. All-in-all it appears the author of that library had percentile
and quantile confused. At some point in the last year, somebody did add a
`ValueAtPercentile` method which has the identical behavior as
`ValueAtQuantile`.

See https://github.com/cockroachdb/vendored/blob/5b815c7d468d59337db2c742373183af5525e729/github.com/codahale/hdrhistogram/hdr.go#L241-L259

Fixes cockroachdb#72282.

Release note: None
@ajwerner ajwerner requested review from nvanbenschoten and a team November 1, 2021 15:22
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: 🤦, nice catch.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)

@ajwerner
Copy link
Contributor Author

ajwerner commented Nov 1, 2021

TFTR!

bors r=nvanbenschoten

Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh my

@craig
Copy link
Contributor

craig bot commented Nov 1, 2021

Build succeeded:

@craig craig bot merged commit ce622df into cockroachdb:master Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

roachtest,workload/tpcc: tpccbench does not taking latency criteria into consideration
4 participants