sql: add more testing for concurrent region/rbr interactions #63804
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajstorm and @arulajmani)
pkg/ccl/multiregionccl/region_test.go, line 478 at r1 (raw file):
_, err := sqlDB.Exec(regionAlterCmd.cmd) if regionAlterCmd.shouldSucceed { require.NoError(t, err)
t.Fatal on a goroutine is apparently a no-no from my research, can we send an error to a waiting channel to be checked by the main channel?
pkg/ccl/multiregionccl/region_test.go, line 481 at r1 (raw file):
} else { if !testutils.IsError(err, "boom") { t.Errorf("expected error boom, found %v", err)
probably should be the equivalent of require.Truef(t, testutils.IsError(err, "boom") ), but goroutine so should send it out.
pkg/ccl/multiregionccl/region_test.go, line 484 at r1 (raw file):
} } typeChangeFinished <- struct{}{}
nit: can we defer this to prevent stuck routines? also, do you mind using close instead of send empty struct?
pkg/ccl/multiregionccl/region_test.go, line 536 at r1 (raw file):
return nil }) <-typeChangeFinished
nit: can we defer this close
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/multiregionccl/region_test.go, line 484 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
nit: can we defer this to prevent stuck routines? also, do you mind using close instead of send empty struct?
(i.e. defer func() { close (typeChangeFinished) }() at the beginning of the goroutine)
pkg/ccl/multiregionccl/region_test.go, line 536 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
nit: can we defer this close
oops, ignore this comment!
Informs cockroachdb#63462 This patch adds TestRegionAddDropEnclosingRegionalByRowOps, which contains subtests with the following sketch: - Client 1 performs an ALTER ADD / DROP REGION. Let the user txn commit. - Block in the type schema changer. - Client 2 performs an operation on a REGIONAL BY ROW table, such as creating an index, unique constraint, altering the primary key etc. - Test both a rollback in the type schema changer and a successful execution. - Ensure the partitions on the REGIONAL BY ROW table are sane. Release note: None
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/multiregionccl/region_test.go, line 478 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
t.Fatal on a goroutine is apparently a no-no from my research, can we send an error to a waiting channel to be checked by the main channel?
I'll just switch to using t.Error which I know is encouraged over t.Fatal in goroutines.
pkg/ccl/multiregionccl/region_test.go, line 481 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
probably should be the equivalent of
require.Truef(t, testutils.IsError(err, "boom") ), but goroutine so should send it out.
Errorf is fine, a linter told me once.
pkg/ccl/multiregionccl/region_test.go, line 484 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
(i.e. defer func() { close (typeChangeFinished) }() at the beginning of the goroutine)
Done!
|
CI failure is unlreated, gonna merge. TFTR! bors r=otan |
|
Build failed (retrying...): |
|
Build succeeded: |
Informs #63462
This patch adds TestRegionAddDropEnclosingRegionalByRowOps, which
contains subtests with the following sketch:
an index, unique constraint, altering the primary key etc.
Release note: None