-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
schemachanger: add doc.go #104780
schemachanger: add doc.go #104780
Conversation
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. |
cc @rafiss @Xiang-Gu @chengxiong-ruan @fqazi for some proofreading please |
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.
looks very good to me. thanks for writing this. I personally probably know most of the stuff, but never be able to describe them systematically like this.
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.
-
somewhere in the doc, preferably in a notable place, we should explain why it's called "declarative" schema changer.
-
I think all the efforts we made into testing this framework also deserves a section. In my opinion, it's quite sophisticated and showcases the testability of the framework -- end-to-end, rollback, puase/resume, backup/restore, DML injection, three explains, and mixed version tests.
Those are all excellent comments, thank you very much for having given this a proper read-through! |
Fixes cockroachdb#104779. Release note: None
I've applied almost all of the suggestions, thanks again! bors r+ |
Build succeeded: |
Fixes #104779.
Release note: None