Skip to content
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

scrun: added a cluster setting to skip planner sanity checks in prod #92774

Merged

Conversation

Xiang-Gu
Copy link
Contributor

Previously, there is a niche case where we might run into a backward incompatible issue (see #91528). We decided to fix it by relaxing the sanity check that caused the issue and backport the change to relase-22.2, so this backward incompatibility issue is contained only between release-22.2.0 and master, which we think should be a rare occurrance in the wild.

Fixes: #91528
Release note: None

Previously, there is a niche case where we might run into a backward
incompatible issue (see cockroachdb#91528). We decided to fix it by relaxing the
sanity check that caused the issue and backport the change to
relase-22.2, so this backward incompatibility issue is contained only
between release-22.2.0 and master, which we think should be a rare
occurrance in the wild.

Epic: None
Release note: None
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@postamar postamar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to test this? I'm concerned of the possibility that while BuildStages no longer throws an error now, ValidateStages might still be.

@Xiang-Gu
Copy link
Contributor Author

Xiang-Gu commented Dec 5, 2022

The only way to test I can think of is in a roachtest; in fact, we do have such a roachtest but it's currently disabled (It will be revived soon when build version is v23.1).

I tested it manually:

  • Apply this PR to both master and release-22.2.
  • Then do the following commands:
roachprod create local -n 2
git checkout release-22.2
dev build cockroach
mv cockroach cockroach-22.2
roachprod put local:1 ./cockroach-22.2
git checkout master
dev build cockroach
roachprod put local:2 ./cockroach
roachprod start local:1 --binary=cockroach-22.2
roachprod start local:2 --binary=cockroach
roachprod run local:2 -- touch data/DISABLE_STARTING_BACKGROUND_JOBS
roachprod sql local:2 -- -e 'create table t (i int primary key);'
roachprod sql local:2 -- -e 'drop table t;'

The last command to drop the table succeeded, which means the job for drop table t is backward compatible and can be adopted and completed by a node running v22.2:
Screen Shot 2022-12-05 at 10 19 41 AM

(Note that it took >30s to finish that last command; I suspect it's related to how we detect and adopt pending jobs in our framework, instead of the command itself taking that long)

@Xiang-Gu Xiang-Gu marked this pull request as ready for review December 5, 2022 17:47
@Xiang-Gu Xiang-Gu requested a review from a team December 5, 2022 17:47
Copy link
Contributor

@postamar postamar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I forgot about this PR! Approved, of course.

(Note that it took >30s to finish that last command; I suspect it's related to how we detect and adopt pending jobs in our framework, instead of the command itself taking that long)

I agree. I noticed the same thing when setting pausepoints in demo.

@Xiang-Gu
Copy link
Contributor Author

Xiang-Gu commented Dec 8, 2022

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 8, 2022

Build failed (retrying...):

@craig craig bot merged commit fe683e3 into cockroachdb:master Dec 8, 2022
@craig
Copy link
Contributor

craig bot commented Dec 8, 2022

Build succeeded:

@blathers-crl
Copy link

blathers-crl bot commented Dec 8, 2022

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 255ceba to blathers/backport-release-22.2-92774: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql/schemachanger: declarative state generated on gc_job_mixed is not backwards compatible
3 participants