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

cmd/workload: add fixtures subcommand #21373

Merged
merged 2 commits into from
Jan 22, 2018

Conversation

danhhz
Copy link
Contributor

@danhhz danhhz commented Jan 10, 2018

This commit adds tooling for a central repository of precomputed test
fixtures corresponding to different configurations of
workload.Generators.

To list all available fixtures:

workload fixtures list

To load a fixture into a CockroachDB cluster:

workload fixtures load <generator> <generator flags> [crdb url]

To compute and store a fixture (the cluster is used to distribute work):

workload fixtures load --into_db=<db> <generator> <generator flags> [crdb url]

workload <generator> has been moved to workload run <generator> so
we can have subcommands on workload.

Still TODO is a better integration with workload run, which currently
always recreates initial data (and does this via the much slower
workload.Setup). There is also lots of opportunity for parallelism in
the fixture loading and storing, but I wanted to get the basics working
first.

@danhhz danhhz requested review from tbg and petermattis January 10, 2018 16:46
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg
Copy link
Member

tbg commented Jan 11, 2018

Looks pretty fabulous! Left some comments.


Reviewed 2 of 2 files at r1, 10 of 10 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


pkg/cmd/workload/fixtures.go, line 63 at r2 (raw file):

	`'gcloud auth application-default login --project=cockroach-shared')`

// getStorge returns a GCS client using "application default" credentials. The

Storge


pkg/cmd/workload/fixtures.go, line 86 at r2 (raw file):

			crdb := crdbDefaultURI
			if len(args) > 0 {
				crdb = args[0]

Error if len(args) > 1?


pkg/testutils/workload/fixture.go, line 86 at r2 (raw file):

func generatorToGCSFolder(store FixtureStore, gen Generator) string {
	meta := gen.Meta()

It feels that the CockroachDB (cluster) version that created the fixture should be part of the key. This makes a difference, for example for #20554 I really want to be able to import a large fixture that predates my fix for that issue to observe how the stats get updated.


pkg/testutils/workload/fixture.go, line 185 at r2 (raw file):

		)
		if _, err := sqlDB.ExecContext(ctx, importStmt, csvURI, backupURI); err != nil {
			return Fixture{}, errors.Wrapf(err, `creating backup for table %s`, table.Name)

I've been staring at this method and feel that it could really use some commentary. It seems that you're putting a CSV file into GCS and then that IMPORT statement really creates the backup (the temp=$2 option) as a side effect. This isn't obvious (but cool), so mention it please.

Why do you keep the csv file?


pkg/testutils/workload/fixture.go, line 189 at r2 (raw file):

		backups = append(backups, FixtureTable{TableName: table.Name, BackupURI: backupURI})
	}
	return Fixture{Store: store, Generator: gen, Tables: backups}, nil

You could return GetFixture(...) instead, which is a little more robust.


pkg/testutils/workload/fixture.go, line 196 at r2 (raw file):

func RestoreFixture(ctx context.Context, sqlDB *gosql.DB, fixture Fixture, database string) error {
	for _, table := range fixture.Tables {
		importStmt := fmt.Sprintf(`RESTORE csv.%s FROM $1 WITH into_db=$2`, table.TableName)

What is this csv. database? Is that some magic? Add a comment here too, please.


pkg/testutils/workload/workload.go, line 69 at r2 (raw file):

It should be bumped whenever InitialRowFn or InitialRowCount change.


Comments from Reviewable

@tbg
Copy link
Member

tbg commented Jan 12, 2018

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


pkg/testutils/workload/fixture.go, line 86 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

It feels that the CockroachDB (cluster) version that created the fixture should be part of the key. This makes a difference, for example for #20554 I really want to be able to import a large fixture that predates my fix for that issue to observe how the stats get updated.

Actually I'm not sure about my earlier statement. The stats are recomputed as you restore, and the backup format is intentionally not changing between CockroachDB versions, so that it shouldn't matter.


Comments from Reviewable

@danhhz danhhz force-pushed the workload_fixture branch 3 times, most recently from 0c6a80d to b5a18e5 Compare January 22, 2018 15:57
This commit adds tooling for a central repository of precomputed test
fixtures corresponding to different configurations of
`workload.Generator`s.

To list all available fixtures:

    workload fixtures list

To load a fixture into a CockroachDB cluster:

    workload fixtures load <generator> <generator flags> [crdb url]

To compute and store a fixture (the cluster is used to distribute work):

    workload fixtures load --into_db=<db> <generator> <generator flags> [crdb url]

`workload <generator>` has been moved to `workload run <generator>` so
we can have subcommands on `workload`.

Still TODO is a better integration with `workload run`, which currently
always recreates initial data (and does this via the much slower
workload.Setup). There is also lots of opportunity for parallelism in
the fixture loading and storing, but I wanted to get the basics working
first.

Release note: None
@danhhz
Copy link
Contributor Author

danhhz commented Jan 22, 2018

Review status: 0 of 10 files reviewed at latest revision, 7 unresolved discussions.


pkg/cmd/workload/fixtures.go, line 63 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Storge

Done.


pkg/cmd/workload/fixtures.go, line 86 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Error if len(args) > 1?

cobra.RangeArgs(0, 1) handles this : - )


pkg/testutils/workload/fixture.go, line 86 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Actually I'm not sure about my earlier statement. The stats are recomputed as you restore, and the backup format is intentionally not changing between CockroachDB versions, so that it shouldn't matter.

👍 yeah, i'd prefer to keep the cockroach version out of it if possible, since a big part of this plan is that some of the fixtures will be expensive to recompute, so we don't want to do it for every cockroach version


pkg/testutils/workload/fixture.go, line 185 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

I've been staring at this method and feel that it could really use some commentary. It seems that you're putting a CSV file into GCS and then that IMPORT statement really creates the backup (the temp=$2 option) as a side effect. This isn't obvious (but cool), so mention it please.

Why do you keep the csv file?

Oh yeah good point. Done. Leaving csv file cleanup as a TODO


pkg/testutils/workload/fixture.go, line 189 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

You could return GetFixture(...) instead, which is a little more robust.

Done.


pkg/testutils/workload/fixture.go, line 196 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

What is this csv. database? Is that some magic? Add a comment here too, please.

Done.


pkg/testutils/workload/workload.go, line 69 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

It should be bumped whenever InitialRowFn or InitialRowCount change.

Done.


Comments from Reviewable

@tbg
Copy link
Member

tbg commented Jan 22, 2018

:lgtm:. You're aware of #21490? I think that changes syntax you're using.


Reviewed 1 of 2 files at r1, 10 of 10 files at r3, 10 of 10 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


pkg/cmd/workload/fixtures.go, line 86 at r2 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

cobra.RangeArgs(0, 1) handles this : - )

Ah, nice! Carry on.


Comments from Reviewable

@danhhz
Copy link
Contributor Author

danhhz commented Jan 22, 2018

Ah, I knew something like that was coming, but hadn't caught up enough on email to see the actual PR. I'll ping it to give a heads up. TFTR!

@danhhz danhhz merged commit c14c3ba into cockroachdb:master Jan 22, 2018
@danhhz danhhz deleted the workload_fixture branch January 22, 2018 17:44
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