Skip to content

workload/tpcc: perform checks atomically, optionally historically and separately #39095

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

Merged
merged 2 commits into from
Jul 30, 2019

Conversation

ajwerner
Copy link
Contributor

This PR comes in two commits. The first makes the TPC-C checks each occur atomically either by using a single transaction or by selecting a timestamp and running all statements at that timestamp using AOST. In addition that commit allows the checks to be run historically.

The second commit adds a workload Generator to run the checks as a stand-alone workload.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner ajwerner force-pushed the ajwerner/workload-tpcc-checks branch 2 times, most recently from 4fdb1fd to 4b6dac2 Compare July 25, 2019 13:01
Copy link
Contributor

@danhhz danhhz left a comment

Choose a reason for hiding this comment

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

:lgtm: Cool!

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)


pkg/workload/tpcc/checks_generator.go, line 33 at r2 (raw file):

An --as-of flag is exposed to prevent the work from interfering with a
foreground TPC-C workload`,
	Version:      `19.2.0`,

This is a version for the workload, not for crdb. It's used to detect backwards incompatible changes to schema, etc when we persist a workload (think fixtures), which doesn't apply to this one since it doesn't have its own data. I'd either omit it or set it to "1.0.0" like most of the other workloads, so it doesn't get confused for a crdb version.


pkg/workload/tpcc/checks_generator.go, line 34 at r2 (raw file):

foreground TPC-C workload`,
	Version:      `19.2.0`,
	PublicFacing: true,

If this doesn't have a specific reason to be built into the cockroach binary, then let's move it to another package (something like pkg/workload/tpcc/tpccchecks?). If it does, then let's get rid of this PublicFacing, which is reserved for only a very select few workloads that are particularly interesting to our users.


pkg/workload/tpcc/checks_generator.go, line 45 at r2 (raw file):

			`Number of concurrent workers. Defaults to 1.`,
		)
		g.flags.StringVar(&g.asOfSystemTime, "as-of", "",

this should also be listed as a RuntimeOnly flag in the flags meta.


pkg/workload/tpcc/checks_generator.go, line 83 at r2 (raw file):

	asOfSystemTime string
	checks         []string
	tolerateErrors bool

unused?


pkg/workload/tpcc/checks_generator.go, line 118 at r2 (raw file):

	checks, err := filterChecks(allChecks(), w.checks)

	// Preregister all of the histograms so they always print.

Is this comment accurate? I think it's meant to go below

@ajwerner ajwerner force-pushed the ajwerner/workload-tpcc-checks branch from 4b6dac2 to 741cc73 Compare July 30, 2019 12:24
Before this commit the TPCC checks required load to be stopped before running
because individual statements were not run in the same transaction or at the
same logical timestamp.

This PR addresses the atomicity issues and also adds functionality to perform
checks at a historical timestamp. This is useful when running the checks
concurrently with the benchmark to create an overload scenario that does not
directly interfere with foreground transactions.

Release note: None
@ajwerner ajwerner force-pushed the ajwerner/workload-tpcc-checks branch from 741cc73 to ab87fe1 Compare July 30, 2019 13:11
@ajwerner ajwerner requested a review from a team July 30, 2019 13:11
This commit adds a workload to run tpcc checks as a separate workload.
This workload is an easy way to create an overload scenario. It can be run with
`--as-of` at a historical timestamp to avoid interferring with foreground
transactions.

Although the hope was that this test could be used to starve node liveness, in
practice it seems to kill nodes by OOM.

Release note: None
@ajwerner ajwerner force-pushed the ajwerner/workload-tpcc-checks branch from ab87fe1 to 04013f7 Compare July 30, 2019 13:13
Copy link
Contributor Author

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/workload/tpcc/checks_generator.go, line 33 at r2 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

This is a version for the workload, not for crdb. It's used to detect backwards incompatible changes to schema, etc when we persist a workload (think fixtures), which doesn't apply to this one since it doesn't have its own data. I'd either omit it or set it to "1.0.0" like most of the other workloads, so it doesn't get confused for a crdb version.

Done.


pkg/workload/tpcc/checks_generator.go, line 34 at r2 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

If this doesn't have a specific reason to be built into the cockroach binary, then let's move it to another package (something like pkg/workload/tpcc/tpccchecks?). If it does, then let's get rid of this PublicFacing, which is reserved for only a very select few workloads that are particularly interesting to our users.

Done.


pkg/workload/tpcc/checks_generator.go, line 45 at r2 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

this should also be listed as a RuntimeOnly flag in the flags meta.

Done.


pkg/workload/tpcc/checks_generator.go, line 83 at r2 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

unused?

Done.


pkg/workload/tpcc/checks_generator.go, line 118 at r2 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

Is this comment accurate? I think it's meant to go below

Done.

Copy link
Contributor Author

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

TFTR!

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)

craig bot pushed a commit that referenced this pull request Jul 30, 2019
39095: workload/tpcc: perform checks atomically, optionally historically and separately r=ajwerner a=ajwerner

This PR comes in two commits. The first makes the TPC-C checks each occur atomically either by using a single transaction or by selecting a timestamp and running all statements at that timestamp using AOST. In addition that commit allows the checks to be run historically.

The second commit adds a workload Generator to run the checks as a stand-alone workload.

Co-authored-by: Andrew Werner <[email protected]>
@craig
Copy link
Contributor

craig bot commented Jul 30, 2019

Build succeeded

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.

3 participants