ttl: replace SPLIT AT with SplitTable in tests #86349
Conversation
|
This change is |
pkg/sql/ttl/ttljob/ttljob.go
Outdated
| if expectedNumSpanPartitions != 0 { | ||
| actualNumSpanPartitions := len(spanPartitions) | ||
| if expectedNumSpanPartitions != actualNumSpanPartitions { | ||
| return errors.Newf( |
There was a problem hiding this comment.
should we make this AssertionFailedf which fails a bit more loudly?
| @@ -46,6 +47,8 @@ import ( | |||
| "github.com/stretchr/testify/require" | |||
| ) | |||
|
|
|||
| var zeroDuration time.Duration | |||
There was a problem hiding this comment.
wonder if you can make this const - const zeroDuration = time.Duration(0)
There was a problem hiding this comment.
It looks like that doesn't help since we would need to assign the const to a var to get its address.
https://stackoverflow.com/questions/35146286/find-address-of-constant-in-go
pkg/sql/ttl/ttljob/ttljob_test.go
Outdated
| )) | ||
|
|
||
| // split table | ||
| splitAt := 10_000 |
There was a problem hiding this comment.
should we make splitAt and rowsPerRange const?
pkg/sql/ttl/ttljob/ttljob_test.go
Outdated
| @@ -88,12 +92,18 @@ func newRowLevelTTLTestJobTestHelper( | |||
|
|
|||
| // As `ALTER TABLE ... SPLIT AT ...` is not supported in multi-tenancy, we | |||
There was a problem hiding this comment.
this might be a problem i introduced, but this comment seems dated. maybe "we only run tests against a tenant for tests not utilising SPLIT AT". which your change, do multi-tenant tests work?
There was a problem hiding this comment.
It gives this error
ttljob_test.go:389: error executing 'SHOW RANGES FROM TABLE tbl': pq: RangeIterator failed to seek to /Meta2/"\x00": rpc error: code = Unauthenticated desc = requested key /Meta2/"\x00" not fully contained in tenant keyspace /Tenant/1{0-1}
pkg/sql/exec_util.go
Outdated
| RequireMultipleSpanPartitions bool | ||
| // ExpectedNumSpanParitions causes the TTL job to fail if it does not match | ||
| // the number of DistSQL processors. | ||
| ExpectedNumSpanParitions int |
refs #85800 SPLIT AT was causing occasional test failures if it did not asynchronously split the range before the TTL job started. SplitTable synchronously splits the range before the test starts the TTL job. Release justification: Fix test flake. Release note: None
|
bors r=otan |
Yeah it seems in general tests should be using the synchronous version to make the behavior deterministic. |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
refs #85800
fixes #86376
SPLIT AT was causing occasional test failures if it did not asynchronously
split the range before the TTL job started. SplitTable synchronously splits
the range before the test starts the TTL job.
Release justification: Fix test flake.
Release note: None