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/indexes: support multi-region aws and tune for latency #53390

Merged

Conversation

nvanbenschoten
Copy link
Member

This PR updates the indexes suite of roachtests to support a multi-region AWS deployment alongside its multi-region GCE deployment.

The commit then tunes the workload to optimize for a stable p50 and p99 latency all the way up to 50 secondary indexes. The concurrency is fairly low with low numbers of indexes, but that's not what the test is trying to stress.

This roachtest with these changes is being used in a POC to demonstrate single-round trip, multi-range transactions.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@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.

:lgtm:

Reviewed 2 of 2 files at r1, 1 of 1 files at r2, 2 of 2 files at r3.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)


pkg/cmd/roachtest/indexes.go, line 70 at r3 (raw file):

					// Add a small delay to allow ranges to rebalance.
					t.l.Printf("waiting for rebalancing")
					time.Sleep(time.Duration(5*secondaryIndexes) * time.Second)

consider polling the replication reports rather than all of this?

Using composite indexes, we increase the range of secondary indexes
from [0-9] to [0-99].
This has always seemed a little low, and it becomes a big issue with
high latency replication with fan-out batches. For instance, when
writing to a table with many secondary indexes.
This commit updates the `indexes` suite of roachtests to support a multi-region
AWS deployment alongside its multi-region GCE deployment.

The commit then tunes the workload to optimize for a stable p50 and p99 latency
all the way up to 50 secondary indexes. The concurrency is fairly low with low
numbers of indexes, but that's not what the test is trying to stress.
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/workloadManyIndexes branch from cccd8d9 to ac89b99 Compare September 15, 2020 20:11
Copy link
Member Author

@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.

TFTR!

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


pkg/cmd/roachtest/indexes.go, line 70 at r3 (raw file):

Previously, ajwerner wrote…

consider polling the replication reports rather than all of this?

Done. @andreimatei might also want to take a look at this. Replication reports aren't quite as easy to use as I hoped. For one, there's no way to trigger one on demand (that I can tell). Worse, it's pretty tricky to guarantee that a report is observing a newly added table or zone (again, that I can tell). They're also completely blind to lease preferences, so they can't be used to wait for lease-partitioning topologies to converge. Instead, we still need to poll crdb_internal.ranges like we do below. But this is difficult to generalize as leaseholder preference configurations become more complex.

Is the new commit using replication reports how you'd expect them to be used and how you'd like them to be used in other roachtests as well? I'm asking in part because I'd like to address #44999 during this stability period.

This is also something we should keep on our minds as we start another push towards improving the global experience.

This commit updates the `indexes` suite of roachtests to use replication
reports to wait for proper rebalancing across regions instead of waiting
for an estimated amount of time.
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/workloadManyIndexes branch from ab100ca to b6ba340 Compare September 16, 2020 14:03
@nvanbenschoten
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 23, 2020

Build succeeded:

@craig craig bot merged commit b618906 into cockroachdb:master Oct 23, 2020
@nvanbenschoten nvanbenschoten deleted the nvanbenschoten/workloadManyIndexes branch December 2, 2020 01:03
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