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

roachtest: add schemachange/bulkingest test #36999

Merged
merged 1 commit into from
Apr 29, 2019

Conversation

thoszhang
Copy link
Contributor

Add a test to index the random payload column for the bulkingest workload,
to test the index backfiller for an index on randomly ordered values relative
to the primary key.

Release note: None

@thoszhang thoszhang requested review from dt and vivekmenezes April 22, 2019 19:28
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@vivekmenezes vivekmenezes left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together. I think I'd prefer this test be in the schemachange.go file to keep with convention. Thanks

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dt, @lucy-zhang, and @vivekmenezes)


pkg/cmd/roachtest/bulkingest.go, line 36 at r1 (raw file):

func runSchemaChangeBulkIngest(ctx context.Context, t *test, c *cluster) {
	aNum := 100000000

Can you add a comment here discussing these parameters?


pkg/cmd/roachtest/bulkingest.go, line 81 at r1 (raw file):

			pauseDuration = time.Second * 10
		}
		time.Sleep(pauseDuration)

what's this pause for? What about if you running locally? perhaps we should put all the parameters at the top and set them based on a size control param. That way someone can easily run this test with larger/smaller inputs based on the value of the size control param


pkg/cmd/roachtest/bulkingest.go, line 88 at r1 (raw file):

		}
		c.l.Printf("CREATE INDEX took %v\n", timeutil.Since(before))
		return nil

This test needs a timeout somewhere.

@thoszhang thoszhang force-pushed the bulkingest-roachtest branch 3 times, most recently from 69bb89d to 4b2214b Compare April 24, 2019 20:17
Copy link
Contributor Author

@thoszhang thoszhang left a comment

Choose a reason for hiding this comment

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

Moved to schemachange.go.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dt and @vivekmenezes)


pkg/cmd/roachtest/bulkingest.go, line 36 at r1 (raw file):

Previously, vivekmenezes wrote…

Can you add a comment here discussing these parameters?

Done.


pkg/cmd/roachtest/bulkingest.go, line 81 at r1 (raw file):

Previously, vivekmenezes wrote…

what's this pause for? What about if you running locally? perhaps we should put all the parameters at the top and set them based on a size control param. That way someone can easily run this test with larger/smaller inputs based on the value of the size control param

It's just so that we wait a few minutes while the loadgen runs before building the index. I updated it so that it only sleeps for a non-local cluster and added a comment.

I moved all the interesting parameters to the function arguments (not including the sleep interval) to match the other tests, which I think is sufficient - it's hard to have a more general size control parameter since the number of nodes and the number of rows are independent of each other and have different effects on behavior.


pkg/cmd/roachtest/bulkingest.go, line 88 at r1 (raw file):

Previously, vivekmenezes wrote…

This test needs a timeout somewhere.

Done.

Add a test to index the random `payload` column for the `bulkingest` workload,
to test the index backfiller for an index on randomly ordered values relative
to the primary key.

Release note: None
@thoszhang thoszhang force-pushed the bulkingest-roachtest branch from 4b2214b to 36f26e4 Compare April 25, 2019 17:23
Copy link
Contributor

@vivekmenezes vivekmenezes 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 (waiting on @dt)

@thoszhang
Copy link
Contributor Author

bors r+

craig bot pushed a commit that referenced this pull request Apr 29, 2019
36744: storage/rangefeed: use fine-grained locking around Processor, add metrics r=nvanbenschoten a=nvanbenschoten

It is critical for correctness that operations like providing the processor
with logical ops or informing the processor of closed timestamp updates
be properly synchronized via the raftMu. This ensures that events published
on the processors eventC accurately reflect the order of events in the Raft
log.

However, it is not critical that lifecycle events around starting a
Rangefeed processor, stopping a Rangefeed processor, and registering
with a Rangefeed processor be synchronized with Raft application. This
change exploits this to break up the locking around rangefeed.Processor.

Using more fine-grained locking opens up the opportunity to interact
with the rangefeed Processor without needing to synchronize with the
Range's raft application, which can be very expensive. The next commit
uses this improvement to add a new `rangefeed_registrations` metric to
`RangeInfo`.

36999: roachtest: add schemachange/bulkingest test r=lucy-zhang a=lucy-zhang

Add a test to index the random `payload` column for the `bulkingest` workload,
to test the index backfiller for an index on randomly ordered values relative
to the primary key.

Release note: None

37169: opt: don't use delegateQuery for ShowZoneConfig r=RaduBerinde a=RaduBerinde

Part of a larger work item to remove `delegateQuery`, as it doesn't
work with the optimizer.

I could not use the new delegate infrastructure because it doesn't support hiding columns returned by the delegated query.

Release note: None

37182: roachtest: provision more cpu for schemachange tpcc tests r=vivekmenezes a=vivekmenezes

related to 
#36094 
#36024 
#36321 

Release note: None

Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Lucy Zhang <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>
Co-authored-by: Vivek Menezes <[email protected]>
@craig
Copy link
Contributor

craig bot commented Apr 29, 2019

Build succeeded

@craig craig bot merged commit 36f26e4 into cockroachdb:master Apr 29, 2019
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.

4 participants