-
Notifications
You must be signed in to change notification settings - Fork 4.1k
release-24.3: schemachangerccl: split backup tests #160578
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
Conversation
|
Thanks for opening a backport. Before merging, please confirm that it falls into one of the following categories (select one):
Add a brief release justification to the PR description explaining your selection. Also, confirm that the change does not break backward compatibility and complies with all aspects of the backport policy. All backports must be reviewed by the TL and EM for the owning area. |
|
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
✅ PR #160578 is compliant with backport policy Confidence: high 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
b3a8d24 to
e94b212
Compare
rafiss
left a comment
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 had a few questions/comments:
- Do you know why this had to be done differently than #159914 ?
- Should we get this into 25.2 and 25.3 also?
- I thought we were discussing only backporting #160344 to skip the restore_all_tables case? why is this splitting PR needed too?
- nit: this PR names the package
scbackuptestmrccl, but on master it's namedsctestbackupmrccl
Previously, all of the CCL tests would run in a single package. We would observe test flakes the global timeout for the package was too low. To address this, this package shards the backup tests similar to newer branches. Fixes: cockroachdb#160455 Release note: None
e94b212 to
933f00f
Compare
The 24.3 branch was missing enhancements to split the backup tests in general, so part of the changes is to split the backup tests for single region, and a minor changes to make MultiRegionTestClusterFactory accessible.
So, there is a manual backport that adds a split on 25.3: #160574. I'll use that to get one on 25.2 as well.
The number of multiregion tests isn't really large on 24.3, we have about 11 of them. The most complex schema change CREATE INDEX which is 7 stages, so I'm not sure if it would buy us too much versus better sharding. I think we can avoid the restore_all_tables changes on this branch if the sharding is sufficient. Let me know what you think.
Fixed |
I see, in that case it seems fine to only do the |
|
@rafiss TFTR! |
Previously, all of the CCL tests would run in a single package. We would observe test flakes the global timeout for the package was too low. To address this, this package shards the backup tests similar to newer branches.
Fixes: #160455
Release note: None
Release justification: test only change