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

roachtest: add CREATE INDEX on tpcc 1000 cluster #31319

Merged
merged 1 commit into from Oct 16, 2018

Conversation

Projects
None yet
5 participants
@vivekmenezes
Contributor

vivekmenezes commented Oct 12, 2018

The roachtest creates an index against a table with
30M rows on a tpcc cluster with a 1000 warehouses.

The test takes around 3 hours to run.

Release note: None

@vivekmenezes vivekmenezes requested a review from dt Oct 12, 2018

@cockroach-teamcity

This comment has been minimized.

Show comment
Hide comment
@cockroach-teamcity

cockroach-teamcity Oct 12, 2018

Member

This change is Reviewable

Member

cockroach-teamcity commented Oct 12, 2018

This change is Reviewable

@petermattis

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/cmd/roachtest/create_index.go, line 28 at r1 (raw file):

	numNodes := 5
	r.Add(testSpec{
		Name:    fmt.Sprintf("createindex/tpcc/warehouses=%d/nodes=%d", warehouses, numNodes),

Drive-by comment: the naming of roachtests is becoming inconsistent. We have backupTPCC vs cancel/tpcc vs cdc/w=1000 (which runs TPC-C). We have schemachange and now createindex/... which is also a schema change test. I don't have a proposal here. Perhaps this isn't a big problem. Mainly I'm bringing this up in the hopes of nerd sniping @andreimatei.

roachtest: add CREATE INDEX on tpcc 1000 cluster
The roachtest creates an index against a table with
30M rows on a tpcc cluster with a 1000 warehouses.

The test takes around 3 hours to run.

Release note: None
@dt

dt approved these changes Oct 16, 2018

@vivekmenezes

This comment has been minimized.

Show comment
Hide comment
@vivekmenezes

vivekmenezes Oct 16, 2018

Contributor

bors r+

Contributor

vivekmenezes commented Oct 16, 2018

bors r+

craig bot pushed a commit that referenced this pull request Oct 16, 2018

Merge #31319
31319: roachtest: add CREATE INDEX on tpcc 1000 cluster r=vivekmenezes a=vivekmenezes

The roachtest creates an index against a table with
30M rows on a tpcc cluster with a 1000 warehouses.

The test takes around 3 hours to run.

Release note: None

Co-authored-by: Vivek Menezes <vivek@cockroachlabs.com>
@craig

This comment has been minimized.

Show comment
Hide comment
@craig

craig bot commented Oct 16, 2018

Build succeeded

@craig craig bot merged commit c293d57 into cockroachdb:master Oct 16, 2018

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@petermattis

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/cmd/roachtest/create_index.go, line 28 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Drive-by comment: the naming of roachtests is becoming inconsistent. We have backupTPCC vs cancel/tpcc vs cdc/w=1000 (which runs TPC-C). We have schemachange and now createindex/... which is also a schema change test. I don't have a proposal here. Perhaps this isn't a big problem. Mainly I'm bringing this up in the hopes of nerd sniping @andreimatei.

Ping. I don't want this to get lost.

@andreimatei

This comment has been minimized.

Show comment
Hide comment
@andreimatei

andreimatei Oct 16, 2018

Member

Well I'm not familiar with most of these tests... I'm not sure what you want me to do :)

Member

andreimatei commented Oct 16, 2018

Well I'm not familiar with most of these tests... I'm not sure what you want me to do :)

@petermattis

This comment has been minimized.

Show comment
Hide comment
@petermattis

petermattis Oct 16, 2018

Contributor

Well I'm not familiar with most of these tests... I'm not sure what you want me to do :)

Propose a naming scheme so that we avoid having a dumping ground. Don't you think all of the schema change tests shows be grouped somehow?

Contributor

petermattis commented Oct 16, 2018

Well I'm not familiar with most of these tests... I'm not sure what you want me to do :)

Propose a naming scheme so that we avoid having a dumping ground. Don't you think all of the schema change tests shows be grouped somehow?

@andreimatei

This comment has been minimized.

Show comment
Hide comment
@andreimatei

andreimatei Oct 16, 2018

Member

I do think naming conventions are good for grouping schema change tests in commands like roachtest list. And since I'm getting rid of subtests, I think that names made up of /-separated components is great.
In the case of the test introduced in this patch (createindex/tpcc/warehouses=%d/nodes=%d), I don't really know if this should be considered a schema change test or a tpcc + nemesis test (basically, do we want to emphasize the schema change aspect or the tpcc aspect?).
But I can't really make this determination; for example I don't know what the failing criteria for this test is. I see that the index creation part fails if the create index statements returns an error. I don't know when the TPCC part fails - I see a --tolerate-errors being passed for unexplained reasons, so I assume it doesn't fail much :P.
So, if it's a schema change test, I think it'd prolly be good if the test name started with either schemachange.
If it's a "tpcc test", I think we would have probably benefited from introducing a framework for composing workloads (tpcc) with nemeses (schema change), and expressed the test purely declaratively - and then you'd get an auto-generated name too.

But, like, beyond bike shedding over particulars, I don't feel prepared to philosophize over generalities yet. If you put me in charge of this, I'll respectfully punt.

Member

andreimatei commented Oct 16, 2018

I do think naming conventions are good for grouping schema change tests in commands like roachtest list. And since I'm getting rid of subtests, I think that names made up of /-separated components is great.
In the case of the test introduced in this patch (createindex/tpcc/warehouses=%d/nodes=%d), I don't really know if this should be considered a schema change test or a tpcc + nemesis test (basically, do we want to emphasize the schema change aspect or the tpcc aspect?).
But I can't really make this determination; for example I don't know what the failing criteria for this test is. I see that the index creation part fails if the create index statements returns an error. I don't know when the TPCC part fails - I see a --tolerate-errors being passed for unexplained reasons, so I assume it doesn't fail much :P.
So, if it's a schema change test, I think it'd prolly be good if the test name started with either schemachange.
If it's a "tpcc test", I think we would have probably benefited from introducing a framework for composing workloads (tpcc) with nemeses (schema change), and expressed the test purely declaratively - and then you'd get an auto-generated name too.

But, like, beyond bike shedding over particulars, I don't feel prepared to philosophize over generalities yet. If you put me in charge of this, I'll respectfully punt.

@petermattis

This comment has been minimized.

Show comment
Hide comment
@petermattis

petermattis Oct 17, 2018

Contributor

Given that the author was exercising schema changes, I think the test name should be grouped with other schema change tests. For example, schemachange/createindex/tpcc-%d/nodes=%d.

@vivekmenezes You've been silent on this convo. Can you chime in here?

Contributor

petermattis commented Oct 17, 2018

Given that the author was exercising schema changes, I think the test name should be grouped with other schema change tests. For example, schemachange/createindex/tpcc-%d/nodes=%d.

@vivekmenezes You've been silent on this convo. Can you chime in here?

@vivekmenezes

This comment has been minimized.

Show comment
Hide comment
@vivekmenezes

vivekmenezes Oct 17, 2018

Contributor

@petermattis using the schemachange prefix makes sense at this stage so we at least have some organization at the top level.

Contributor

vivekmenezes commented Oct 17, 2018

@petermattis using the schemachange prefix makes sense at this stage so we at least have some organization at the top level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment