cdctest: add mvcc_timestamp, diff, sink_config options to nemesis testing #139155
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/ccl/changefeedccl/cdctest/validator.go line 388 at r5 (raw file):
} if *mvccJSONText > updated.AsOfSystemTime() {
Wondering if there's a better assertion that we can make here once the mvcc timestamp exists. If I assert that this mvcc timestamp is within up to 5 seconds of the updated timestamp, the tests fail (a few times in 100).
| validators = append(validators, kivV) | ||
| } | ||
|
|
||
| if cfo.BooleanOptions["mvcc_timestamp"] { |
There was a problem hiding this comment.
nit: maybe use the constants in options.go instead of string literals
|
looks reasonable to me. let me know when it's ready for a full review |
aerfrei
force-pushed
the
aerin/randomized-options-2
branch
from
ad8b23c to
78d22d8
Compare
aerfrei
requested review from
asg0451 and
wenyihu6
and removed request for
a team
aerfrei
changed the title
There was a problem hiding this comment.
Should be ready for re-review
Reviewable status:
| } | ||
|
|
||
| retry := make(map[string]any) | ||
| if !isKafka && rand.Intn(2) < 1 { |
There was a problem hiding this comment.
nit: can you clean up this function a bit? a lot of if isKafka && , maybe it can be nested or pulled out into a function that handles kafka specific stuff
| compression := compressions[rand.Intn(len(compressions))] | ||
| base["Compression"] = compression | ||
| if compression == "GZIP" { | ||
| level := rand.Intn(10) |
| } | ||
|
|
||
| jsonData, err := json.Marshal(nonEmptyConfig) | ||
| if err != nil { |
There was a problem hiding this comment.
nit: can you return error here or panic or something?
| return err | ||
| } | ||
|
|
||
| if *mvccJSONText > updated.AsOfSystemTime() { |
There was a problem hiding this comment.
can you also do a sanity check like > 0 or > yesterday?
| } | ||
| keyJSONAsArray, ok := keyJSON.AsArray() | ||
| if !ok || len(keyJSONAsArray) != len(v.primaryKeyCols) { | ||
| return errors.Errorf( |
There was a problem hiding this comment.
should probably append to v.failures here instead of returning error. ditto for (some but not all) other errors in this method
| if err != nil { | ||
| t.Fatalf("got %v expected %v", err, nil) | ||
| } |
There was a problem hiding this comment.
nit: prefer assert.NoError(t, err) or require.NoError(t, err) for these
| if err != nil { | ||
| t.Fatalf("got %v expected %v", err, nil) | ||
| } | ||
| if f := v.Failures(); f != nil { |
There was a problem hiding this comment.
I'm using the assertValidatorFailures now (with no failures) to match the rest of the file
aerfrei
force-pushed
the
aerin/randomized-options-2
branch
from
78d22d8 to
f78327a
Compare
There was a problem hiding this comment.
Reviewable status:
| compression := compressions[rand.Intn(len(compressions))] | ||
| base["Compression"] = compression | ||
| if compression == "GZIP" { | ||
| level := rand.Intn(10) |
| return err | ||
| } | ||
|
|
||
| if *mvccJSONText > updated.AsOfSystemTime() { |
aerfrei
force-pushed
the
aerin/randomized-options-2
branch
from
f78327a to
0f6c777
Compare
There was a problem hiding this comment.
Reviewable status:
| if err != nil { | ||
| t.Fatalf("got %v expected %v", err, nil) | ||
| } | ||
| if f := v.Failures(); f != nil { |
There was a problem hiding this comment.
I'm using the assertValidatorFailures now (with no failures) to match the rest of the file
aerfrei
force-pushed
the
aerin/randomized-options-2
branch
from
0f6c777 to
080fcfc
Compare
…ting This work adds to random changefeed settings we pass into changefeeds in nemesis testing. This commit adds support for: changefeeds without the diff option specified, the mvcc_timestamp option and the kafka and pubsub sink configs. See also: cockroachdb#134119 Epic: CRDB-42866 Release note: None
aerfrei
force-pushed
the
aerin/randomized-options-2
branch
from
080fcfc to
0dc5872
Compare
This work adds to random changefeed settings we pass into changefeeds
in nemesis testing. This commit adds support for: changefeeds without
the diff option specified, the mvcc_timestamp option and the kafka and
pubsub sink configs.
See also: #134119
Epic: CRDB-42866
Release note: None