-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
roachtest: Added a declarative schema changer job compatibility tests #98403
roachtest: Added a declarative schema changer job compatibility tests #98403
Conversation
0c8ee05
to
11ea354
Compare
11ea354
to
f766dd3
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.
This gives good coverage of at least basic sanity between the two releases.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @herkolategan, @srosenberg, and @Xiang-Gu)
-- commits
line 4 at r1:
spelling: revived
typo: ensured => ensures
Code quote:
revivied
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go
line 89 at r1 (raw file):
) versionStep { return func(ctx context.Context, t test.Test, u *versionUpgradeTest) { planAndRunSchemaChange(ctx, t, c, nodeIDs.RandNode(), `DROP SEQUENCE testdb.testsc.s`)
There is overlap here, we can probably have functions that call each other. Also, I'm wondering if master should only test against the previous release, and we backport the roachtest to 22.2
This PR revived a previously skipped roachtest that tests declarative schema changer job compatibility in the mixed version state (V22_2/V23_1).
f766dd3
to
b36c525
Compare
Could you also update the PR description. I was looking for another test for V22_1/V22_2 heh (the commit message is correct :)). Thanks. |
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 (and 1 stale) (waiting on @fqazi, @herkolategan, and @srosenberg)
Previously, fqazi (Faizan Qazi) wrote…
spelling: revived
typo: ensured => ensures
Done.
pkg/cmd/roachtest/tests/mixed_version_job_compatibility_in_declarative_schema_changer.go
line 89 at r1 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
There is overlap here, we can probably have functions that call each other. Also, I'm wondering if master should only test against the previous release, and we backport the roachtest to 22.2
I don't want to separate the overlap here because those stmts have "dependency" (e.g. we should alter a table before dropping it). However, I do like your idea of having only one roachtest on master and backporting the other one to 22.2 (#98541).
@chengxiong-ruan Sorry, I separate that roachtest into a backport PR to 22.2. |
TFTR! bors r+ |
👎 Rejected by code reviews |
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.
LGTM
bors r+ |
👎 Rejected by code reviews |
bors r+ |
This PR was included in a batch that was canceled, it will be automatically retried |
Build succeeded: |
We added a roachtest to test declarative schema change job compatibility, both
forward and backward, in a mixed version state (V22_2/V23_1).
Fixes: #89345
Epic: None
Release Note: None