sql: add tests for concurrent add/drop region operations

[arulajmani]

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

[cockroach-teamcity]

This change is 

[ajstorm]

Reviewed 2 of 2 files at r1.
Reviewable status: 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]

Reviewable status: 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?

[ajstorm]

Reviewed 1 of 1 files at r2.
Reviewable status: 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]

Sorry, should have been a publish and approve!

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

[arulajmani]

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

bors r=ajstorm

Reviewable status: 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]

Yup. I missed the add-drop one. 🚀

…

On Thu, Apr 1, 2021 at 5:54 PM Arul Ajmani ***@***.***> wrote: ***@***.**** commented on this pull request. Discussed offline, we'll unify these error messages in a different PR. TFTR! bors r=ajstorm *Reviewable <https://reviewable.io/reviews/cockroachdb/cockroach/62826#-:-MXEQQkN0jp4cH0cw5h-:b-uis6zb>* status: [image: ] complete! 1 of 0 LGTMs obtained (waiting on @ajstorm <https://github.com/ajstorm> and @arulajmani <https://github.com/arulajmani>) ------------------------------ *pkg/ccl/multiregionccl/region_test.go, line 72 at r1 <https://reviewable.io/reviews/cockroachdb/cockroach/62826#-MX7Cy6ZCHPlAAYieBit:-MXENNQt4l1WdwIwZ-3W:bq00y3a> (raw file <https://github.com/cockroachdb/cockroach/blob/50092a7e067a4a44202272314cd1a720a8261d6a/pkg/ccl/multiregionccl/region_test.go#L72>):* *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? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#62826 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEMXVOTGH5PF7DNCZLUKUR3TGTTSRANCNFSM42C6JAFA> .

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)