Skip to content

sql: add tests for concurrent add/drop region operations#62826

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
arulajmani:concurrent-tests
Apr 2, 2021
Merged

sql: add tests for concurrent add/drop region operations#62826
craig[bot] merged 1 commit intocockroachdb:masterfrom
arulajmani:concurrent-tests

Conversation

@arulajmani
Copy link
Copy Markdown
Collaborator

@arulajmani arulajmani commented Mar 30, 2021

This patch generalizes the setup in what was previously
TestConcurrentDropRegion and extends it to all combinations of
add/drop region on a multi-region database. The only change is that
I've added a regional by row table into the test setup mixer, so as to
excercise the repartitioning semantics.

Previously, there was a limitation with concurrent add/drop regions
where both the operations were bound to fail in the repartitioning
phase. This limitation was fixed in #60620, but we never had a
regression test for it. Adding a regional by row table during the
test setup serves as one.

Closes #62813

Release note: None

@arulajmani arulajmani requested review from a team and ajstorm March 30, 2021 20:55
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Mar 30, 2021

This change is Reviewable

ajstorm
ajstorm requested changes Mar 31, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@ajstorm ajstorm left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)

pkg/ccl/multiregionccl/region_test.go, line 72 at r1 (raw file):

			`ALTER DATABASE db ADD REGION "us-east5"`,
			[]string{"us-east1", "us-east3", "us-east5"},
		},

Really nice! Can we also add test cases for adding/dropping the same region concurrently and generalize this to expect an error for one of the commands?

pkg/ccl/multiregionccl/region_test.go, line 138 at r1 (raw file):

			require.NoError(t, err)
			if numRegions != len(tc.expectedRegions) {
				t.Fatalf("unexpected number of regions, expected: %d found %d",

Will erroring out here make this test case difficult to debug? Is it not possible to validate the count and the regions together below, and if either of them are bad, print out the expected list and what was found?

@arulajmani
sql: add tests for concurrent add/drop region operations
d83b25c 
This patch generalizes the setup in what was previously
`TestConcurrentDropRegion` and extends it to all combinations of
add/drop region on a multi-region database. The only change is that
I've added a regional by row table into the test setup mixer, so as to
excercise the repartitioning semantics.

Previously, there was a limitation with concurrent add/drop regions
where both the operations were bound to fail in the repartitioning
phase. This limitation was fixed in cockroachdb#60620, but we never had a
regression test for it. Adding a regional by row table during the
test setup serves as one.

Closes cockroachdb#62813

Release note: None
arulajmani
arulajmani commented Apr 1, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator Author

@arulajmani arulajmani 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 @ajstorm)

pkg/ccl/multiregionccl/region_test.go, line 72 at r1 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Really nice! Can we also add test cases for adding/dropping the same region concurrently and generalize this to expect an error for one of the commands?

Good call, added!

pkg/ccl/multiregionccl/region_test.go, line 138 at r1 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Will erroring out here make this test case difficult to debug? Is it not possible to validate the count and the regions together below, and if either of them are bad, print out the expected list and what was found?

Is something like this what you had in mind?

pkg/ccl/multiregionccl/region_test.go, line 92 at r2 (raw file):

			`ALTER DATABASE db DROP REGION "us-east2"`,
			[]string{"us-east1", "us-east3"},
			`enum value "us-east2" is already being dropped`,

How do you feel about this error message? I could wrap this around a "cannot drop region" or something of that sort, but before I tack on that commit, do you think it's worth the effort?

@arulajmani arulajmani requested a review from ajstorm April 1, 2021 16:59
ajstorm
ajstorm requested changes Apr 1, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@ajstorm ajstorm left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)

pkg/ccl/multiregionccl/region_test.go, line 72 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Good call, added!

Great. One more minor request. Can you also add one where you swap the order to ensure that we get the reverse error message?

pkg/ccl/multiregionccl/region_test.go, line 138 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Is something like this what you had in mind?

🎉

pkg/ccl/multiregionccl/region_test.go, line 92 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

How do you feel about this error message? I could wrap this around a "cannot drop region" or something of that sort, but before I tack on that commit, do you think it's worth the effort?

Seems to me like the message should refer to the region, and not the enum value. We don't want customers to have to understand the implementation details.

ajstorm
ajstorm approved these changes Apr 1, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@ajstorm ajstorm left a comment

Choose a reason for hiding this comment

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

Sorry, should have been a publish and approve! :lgtm:

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

arulajmani
arulajmani commented Apr 1, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator Author

@arulajmani arulajmani left a comment

Choose a reason for hiding this comment

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

Discussed offline, we'll unify these error messages in a different PR. TFTR!

bors r=ajstorm

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

pkg/ccl/multiregionccl/region_test.go, line 72 at r1 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Great. One more minor request. Can you also add one where you swap the order to ensure that we get the reverse error message?

I've got one for add-drop, drop-add, add-add, and drop-drop. That's all of them, right?

@ajstorm
Copy link
Copy Markdown
Collaborator

ajstorm commented Apr 1, 2021 via email

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Apr 1, 2021

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Apr 2, 2021

Build succeeded:

@craig craig bot merged commit 0966b27 into cockroachdb:master Apr 2, 2021
@arulajmani arulajmani mentioned this pull request Apr 2, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ajstorm ajstorm ajstorm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

sql: add test for concurrent region DDL operations

3 participants

@arulajmani @cockroach-teamcity @ajstorm