-
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
sql: remove PHYSICAL scrub code #74761
Conversation
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.
Although I was never familiar with this SCRUB code or whether it provided value for testing correctness. Might be worth getting another set of eyes too.
Reviewed 28 of 28 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis and @shermanCRL)
8d37664
to
c125447
Compare
The PHYSICAL scrub code is experimental and not considered production ready. It complicates a lot of code paths involved in normal query execution (it significantly overloads the semantics of TableReader and of the Fetcher) and is getting in the way of some improvements in how the fetchers work. In particular, we are trying to reduce the amount of information passed to TableReader/Fetcher (which in the non-scrubbing case should be a lot less than the full table descriptor). There are some proposals for a better design floating around, e.g. provide a facility for returning KVs as results from DistSQL and have some higher-level code run the scrub checks. This change removes the code for the PHYSICAL scrub for now. Release note (sql change): the experimental SCRUB PHYSICAL is no longer implemented.
c125447
to
9dd34a6
Compare
I think I brought up the idea of removing this some time ago, and @jordanlewis was a bit opposed to it. At the time I didn't have any justification other than "it's not being used", but if this code is in the way of pushing more logic into the KV pods, then I'd guess the balance has shifted. It'd be good to get an ack from Jordan though. |
I also brought it up here: https://cockroachlabs.slack.com/archives/CV581CE78/p1637081958010400 |
I have no objection, I can't remember why I was opposed but now seems like a great time to do it. |
TFTRs! bors r+ |
Build failed (retrying...): |
Build succeeded: |
The PHYSICAL scrub code is experimental and not considered production
ready. It complicates a lot of code paths involved in normal query
execution (it significantly overloads the semantics of TableReader and
of the Fetcher) and is getting in the way of some improvements in how
the fetchers work. In particular, we are trying to reduce the amount
of information passed to TableReader/Fetcher (which in the
non-scrubbing case should be a lot less than the full table
descriptor).
There are some proposals for a better design floating around, e.g.
provide a facility for returning KVs as results from DistSQL and have
some higher-level code run the scrub checks.
This change removes the code for the PHYSICAL scrub for now.
Release note (sql change): the experimental SCRUB PHYSICAL is no
longer implemented.