-
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
scbuild: fix concurrent schema change verification bug #106933
Conversation
80f360d
to
b9d73b6
Compare
Previously, we didn't check for concurrent schema changes on targets set on elements that the builder state didn't know about yet. This patch fixes this oversight. This regression was recently introduced by cockroachdb#106175. Fixes: cockroachdb#106487 Release note: None
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.
nice
Thanks for the review. @rafiss has kindly agreed to run the tpce roachtest to validate this change, as I've been unable to do so on my gceworker (reprovisioning it would not be a good idea for other reasons) |
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.
the roachtest is still in progress - in the meantime, i wanted to check if thought was given to trying to write a unit test that reproduces the bug. is there a reason that it would be too much of a challenge?
Reviewable status: complete! 0 of 0 LGTMs obtained
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.
still testing, but looks like my test made it past the part that is currently failing:
15:44:16 tpce.go:63: Installing docker
15:45:00 tpcc.go:1558: test status: setting up prometheus/grafana (<2m0s)
15:45:06 tpce.go:188: test status: initializing 5000 tpc-e customers
17:21:38 tpce.go:205: test status: running workload
17:21:38 cluster.go:2370: > sudo docker run cockroachdb/tpc-e:latest --customers 5000 --duration 2h0m0s --racks 3 --threads 12 --hosts=10.142.0.218 --hosts=10.142.0.221 --hosts=10.142.0.214
i still want us to think about a unit test for this, but if there's a significant barrier to adding one, i'm fine to merge this as is.
Reviewable status: complete! 0 of 0 LGTMs obtained
Thank you very much for the review and the testing! I'll file an issue re. test coverage. bors r+ |
I filed #107223 |
Build succeeded: |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from cd13eb4 to blathers/backport-release-22.2-106933: 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. error creating merge commit from cd13eb4 to blathers/backport-release-23.1-106933: 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 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Previously, we didn't check for concurrent schema changes on targets set
on elements that the builder state didn't know about yet. This patch
fixes this oversight. This regression was recently introduced by #106175.
Fixes: #106487
Release note: None