cdc: Test for falling behind schema TTL #31020
Merged
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 change is |
mrtracy
force-pushed
the
mtracy/cdc_schema_ttl_test
branch
from
25dd012 to
fe51cfc
Compare
danhhz
reviewed
Oct 8, 2018
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (and 1 stale)
pkg/ccl/changefeedccl/helpers_test.go, line 706 at r1 (raw file):
sqlDB *sqlutils.SQLRunner, database, table string, ) {
probably want a t.Helper() at the beginning of this
pkg/ccl/changefeedccl/helpers_test.go, line 711 at r1 (raw file):
stmt = `ALTER DATABASE system CONFIGURE ZONE USING gc.ttlseconds = $1` } else { fmt.Sprintf(`ALTER TABLE %s.%s CONFIGURE ZONE USING gc.ttlseconds = $1`, database, table)
the result of this Sprintf call seems unused
mrtracy
force-pushed
the
mtracy/cdc_schema_ttl_test
branch
2 times, most recently
from
be8f460 to
a006390
Compare
mrtracy
commented
Oct 8, 2018
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/changefeedccl/helpers_test.go, line 706 at r1 (raw file):
Previously, danhhz (Daniel Harrison) wrote…
probably want a t.Helper() at the beginning of this
Done.
pkg/ccl/changefeedccl/helpers_test.go, line 711 at r1 (raw file):
Previously, danhhz (Daniel Harrison) wrote…
the result of this Sprintf call seems unused
You're correct, and so is the linter/failing tests.
mrtracy
commented
Oct 8, 2018
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/changefeedccl/helpers_test.go, line 711 at r1 (raw file):
Previously, mrtracy (Matt Tracy) wrote…
You're correct, and so is the linter/failing tests.
Done.
|
I believe the change to store.go to run a GC is unnecessary. See TestIndexBackfillAfterGC |
|
Hah, thanks Vivek, wish I had found that earlier! I did not think to look for a request, I was only looking for test hooks. This actually means the tests do not need the sleep at all, that's 4 total seconds of test time back. Nice! Finishing up the change now. |
mrtracy
force-pushed
the
mtracy/cdc_schema_ttl_test
branch
from
a006390 to
a504908
Compare
Add a test that ensures that changefeeds properly exit if they fall far enough behind that schema information has been lost due to the GC TTL (that is, a historical row version can no longer be read because the schema at its timestamp has been garbage collected). I have also discovered why the sister test (for the table TTL, not the schema) required a 3 second sleep: the GC queue enforces that replicas must have an appropriately high "score" before being GCed, even when the "shouldQueue" process is skipped. I have swapped to using the "ManuallyEnqueue" method to force the GC, and instead use the already existing GCRequest request type to force a GC of the table; as a result the tests no longer require any sleep. Resolves cockroachdb#28644 Release note: None
mrtracy
force-pushed
the
mtracy/cdc_schema_ttl_test
branch
from
a504908 to
4cab780
Compare
|
bors r=danhhz |
craig bot
pushed a commit
that referenced
this pull request
Oct 9, 2018
30566: sql: asynchronously drop non-interleaved indexes r=eriktrinh a=eriktrinh This change drops non-interleaved indexes asynchronously by performing the deletion of data using an asynchronous schema changer. This is in preparation to eventually remove index data using `ClearRange` after the GC TTL period has passed. The initial schema changer runs through the state machine but does not perform the deletion of index data. Instead the mutation is moved to a separate list and has a timestamp attached. The created asynchronous schema changer uses the timestamp and index's configured GC TTL value to determine when it should begin execution and actually truncate the index. When the async schema changer deletes the index data two things occur: the job is marked as succeeded and the index zone config is removed. The job can immediately be marked as succeeded because currently a separate job is created for each index that is dropped. Interleaved indexes are unaffected and have their data deleted immediately. Related to #20696 Fixes #28859. 31020: cdc: Test for falling behind schema TTL r=danhhz a=mrtracy Add a test that ensures that changefeeds properly exit if they fall far enough behind that schema information has been lost due to the GC TTL (that is, a historical row version can no longer be read because the schema at its timestamp has been garbage collected). I have also discovered why the sister test (for the table TTL, not the schema) required a 3 second sleep: the GC queue enforces that replicas must have an appropriately high "score" before being GCed, even when the "shouldQueue" process is skipped. To get around this, I have changed "ManuallyEnqueueSpan" to a more explicit "ManuallyGCSpan", which directly calls the processing implementation of the gcQueue on the appropriate replicas. Both that sister test, and the new schema TTL test, now only require a more predictable 1 second sleep. Resolves #28644 Release note: None 31152: changefeedccl: fix TestAvroSchema/DECIMAL flake r=mrtracy a=danhhz The precision is really meant to be in [1,10], but it sure looks like there's an off by one error in the avro library that makes this test flake if it picks precision of 1. Release note: None 31154: kubernetes: Add multiregion channel, add channel to daemonset configs r=a-robinson a=a-robinson Release note: None Fixes #31144 Co-authored-by: Erik Trinh <[email protected]> Co-authored-by: Matt Tracy <[email protected]> Co-authored-by: Daniel Harrison <[email protected]> Co-authored-by: Alex Robinson <[email protected]>
Build succeeded |
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.
Add a test that ensures that changefeeds properly exit if they fall far
enough behind that schema information has been lost due to the GC TTL
(that is, a historical row version can no longer be read because the
schema at its timestamp has been garbage collected).
I have also discovered why the sister test (for the table TTL, not the
schema) required a 3 second sleep: the GC queue enforces that replicas
must have an appropriately high "score" before being GCed, even when the
"shouldQueue" process is skipped. To get around this, I have changed
"ManuallyEnqueueSpan" to a more explicit "ManuallyGCSpan", which
directly calls the processing implementation of the gcQueue on the
appropriate replicas. Both that sister test, and the new schema TTL
test, now only require a more predictable 1 second sleep.
Resolves #28644
Release note: None