roachtest: add follower_reads variant using non-voting replicas #59843
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This TODO was added in 2016 and the surrounding structure of the method has entirely changed in the intervening period. It doesn't make sense anymore as we do indeed handle heterogenous sets of replica constraints now. Release note: None
This commit renames the existing `AllocatorAction`s and priorities to indicate that they pertain only to voting replicas. This is intended to pave the way for new non-voter specific allocator actions and priorities that follow in the subsequent commits in this PR. Release note: None
This commit adds basic logic in the allocator to spit out actions that prescribe upreplication or downreplication of non-voting replicas based on zone configs. Note that this commit *does not* enable the `replicateQueue` or the `StoreRebalancer` to actually act on these newly added `AllocatorAction`s, not does it make the allocator smart enough to prescribe _rebalancing_ non-voting replicas. This commit also does not teach the allocator to rank non-voting replicas for addition or removal. Release note: None
This commit primarily teaches the allocator to be able to rank non-voting replica candidates for addition and removal. This allows us to have the replicateQueue execute upon the allocator's actions to add or remove non-voting replicas to a range. Note that this commit does not deal with _rebalancing_ of non-voting replicas, just simple additions and removals when a range is over or under-replicated. Release note: None
Before this commit, the `replicas` column of `crdb_internal.ranges_no_leases` would only included voting replicas. This commit makes it such that it includes voting as well as non-voting replicas. Release note: the `replicas` column of `crdb_internal.ranges_no_leases` now includes both voting and non-voting replicas
This commit adds two new variants to the follower reads roachtest: one that sets up a cluster with 3 voting replicas and 2 non-voting replicas, and another that has 1 voting replica and 2 non-voting replicas. Resolves cockroachdb#59678 Release note: None
|
This change is |
aayushshah15
force-pushed
the
follower_reads_roachtest_with_non_voters-2
branch
from
a1466e4 to
1138421
Compare
nvanbenschoten
approved these changes
Feb 7, 2021
There was a problem hiding this comment.
Cool!
Reviewed 15 of 15 files at r6.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15)
pkg/cmd/roachtest/cluster.go, line 2802 at r6 (raw file):
// TODO(nvanbenschoten): this function should take a context and be responsive // to context cancellation. func waitForFullReplication(t *test, db *gosql.DB) {
nit: consider a waitForFullReplicationWithNumReplicas helper so that you don't have to change all of these call sites.
pkg/cmd/roachtest/follower_reads.go, line 81 at r6 (raw file):
// func runFollowerReadsTest( ctx context.Context, t *test, c *cluster, numVoters int, numNonVoters int,
nit: numVoters, numNonVoters int,
pkg/cmd/roachtest/follower_reads.go, line 213 at r6 (raw file):
t.Fatalf("error verifying node values: %v", err) } // Verify that the follower read count increments on at least n-1 nodes.
Explain why n-1 - because one node is the leaseholder. Maybe also pull numReplicas-1 into a numFollowers variable to make this extra clear.
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
Mar 11, 2021
… tables Fixes cockroachdb#59678. Requires cockroachdb#59839. This commit expands the existing `follower-reads` roachtest into four variants: - follower-reads/survival=region/locality=global - follower-reads/survival=region/locality=regional - follower-reads/survival=zone/locality=global - follower-reads/survival=zone/locality=regional As the names indicate, these four variants are adapted to use the [new multi-region abstractions](https://www.cockroachlabs.com/docs/v21.1/multiregion-overview.html). Specifically, two of the variants configure the test database with a REGION survival goal while two of the variants configure the test database with a ZONE survival goal. The ZONE variant give us testing coverage for serving follower reads off of non-voting replicas, because only the primary region of the database will house voting replicas. The use of multi-region abstractions also extends to table locality settings. Two of the variants configure the table with a REGIONAL locality setting. This necessitates the use of stale reads to read from followers, as the test had been doing before. However, the other two variants configure the table with a GLOBAL locality setting. Because GLOBAL tables can serve non-stale reads from followers, the test can omit the `AS OF SYSTEM TIME follower_read_timestamp()` clause in these variants and test that GLOBAL tables provide low-latency consistent reads from all regions. I remembered a little late that this conflicts with cockroachdb#59843, which adds support for non-voting replicas to this test in a bit of a different way. I'm curious how people feel about these two approaches. I slightly prefer the use of high-level multi-region abstractions instead of lower-level zone configs, because that's what we're going to push people towards going forward and it expands our testing coverage. But if we go with that approach, we should probably address the TODO left here about more carefully checking voting vs. non-voting replica placement.
craig bot
pushed a commit
that referenced
this pull request
Mar 14, 2021
61836: roachtest/follower-reads: adopt multi-region abstractions, add GLOBAL tables r=nvanbenschoten a=nvanbenschoten Fixes #59678. Requires #59839. This commit expands the existing `follower-reads` roachtest into four variants: - `follower-reads/survival=region/locality=global` - `follower-reads/survival=region/locality=regional` - `follower-reads/survival=zone/locality=global` - `follower-reads/survival=zone/locality=regional` As the names indicate, these four variants are adapted to use the [new multi-region abstractions](https://www.cockroachlabs.com/docs/v21.1/multiregion-overview.html). Specifically, two of the variants configure the test database with a REGION survival goal while two of the variants configure the test database with a ZONE survival goal. The ZONE variant give us testing coverage for serving follower reads off of non-voting replicas, because only the primary region of the database will house voting replicas. The use of multi-region abstractions also extends to table locality settings. Two of the variants configure the table with a REGIONAL locality setting. This necessitates the use of stale reads to read from followers, as the test had been doing before. However, the other two variants configure the table with a GLOBAL locality setting. Because GLOBAL tables can serve non-stale reads from followers, the test can omit the `AS OF SYSTEM TIME follower_read_timestamp()` clause in these variants and test that GLOBAL tables provide low-latency consistent reads from all regions. I remembered a little late that this conflicts with #59843, which adds support for non-voting replicas to this test in a bit of a different way. I'm curious how people feel about these two approaches. I slightly prefer the use of high-level multi-region abstractions instead of lower-level zone configs, because that's what we're going to push people towards going forward and it expands our testing coverage. But if we go with that approach, we should probably address the TODO left here about more carefully checking voting vs. non-voting replica placement. Co-authored-by: Nathan VanBenschoten <[email protected]>
|
Closing this PR since #61836 supplants it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First 3 commits from #59389, 4th commit from #59403, 5th commit
from #59839.
This commit adds two new variants to the follower reads roachtest: one
that sets up a cluster with 3 voting replicas and 2 non-voting replicas,
and another that has 1 voting replica and 2 non-voting replicas.
Resolves #59678
Release note: None