-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cdc: test negative timestamp cdc #88058
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great.
Make sure you update PR description, and squash commits before merge.
@@ -328,6 +328,11 @@ func createChangefeedJobRecord( | |||
} | |||
var initialHighWater hlc.Timestamp | |||
evalTimestamp := func(s string) (hlc.Timestamp, error) { | |||
knobs := p.ExecCfg().DistSQLSrv.TestingKnobs.Changefeed.(*TestingKnobs) | |||
if knobs.OverrideCursor != nil { | |||
// Check TestChangefeedCursor for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd drop this comment.
@@ -666,6 +666,7 @@ func TestChangefeedCursor(t *testing.T) { | |||
// 'after', throw a couple sleeps around them. We round timestamps to | |||
// Microsecond granularity for Postgres compatibility, so make the | |||
// sleeps 10x that. | |||
before := s.Server.Clock().Now() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider renaming to beforeInsert
168f797
to
2355d4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @miretskiy)
pkg/ccl/changefeedccl/changefeed_stmt.go
line 333 at r2 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
i'd drop this comment.
Done.
pkg/ccl/changefeedccl/changefeed_test.go
line 669 at r2 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
nit: consider renaming to
beforeInsert
done
pkg/ccl/changefeedccl/changefeed_test.go
line 696 at r2 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
nit: Since this is a placeholder, consider putting really non-sensical
value that's guaranteed to fail the test (if it's not replaced); for example
'-5 days'
done
2355d4d
to
176bf03
Compare
The negative timestamp value is calculated as the difference between a predefined time (this is the time from which we expect changefeed to run) and the current statement time. knobs is used to get the current statement time because it will only be available once the change feed statement starts executing. Resolves: #82350 Release note: None
ec557f4
to
d87876d
Compare
going ahead with merge as github ci test has been failing on master too: |
bors r+ |
Build succeeded: |
The negative timestamp value is calculated as the difference between a
predefined time (this is the time from which we expect changefeed to run)
and the current statement time. knobs is used to get the current statement
time because it will only be available once the change feed statement
starts executing.
Resolves: #82350
Release note: None