-
Notifications
You must be signed in to change notification settings - Fork 59
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
[WIP] Move protocol validation to TableConfiguration #665
base: main
Are you sure you want to change the base?
Conversation
@scovich @nicklan @zachschuermann Error TypesThe introduction of On the testing side, I've already seen improvements in some of the CDF tests. We can now make far stronger assertions on why something fails. Examples: [1], [2] On error communication: I've already seen one incident were a kernel user got an error that didn't explain why they couldn't run CDF on a table. Just that they couldn't. CDF error now explains why CDF is unsupported at that version. The questions are:
Debug AssertionsI want to start using more
I remember that panicing in kernel is especially rough due to FFI, so I'd like to see ways we can catch fatal errors through tracing as well. Newtype pattern for TableConfiguration SemanticsI was experimenting with the NewType pattern to have type-level assertions on what a TableConfiguration supports. @zachschuermann had mentioned he didn't like the previous approach and floated the idea of using types. This is extremely easy with rust's Deref trait, and it avoids the weird semantics we had before where Here's where I used it for Here's the one for Snapshot TableProperty constantsaside: constants won't bind in match statements, so they're a great tool for matching on many strings. let Y = "SOME CONSTANT";
match x {
Y => /* Y here is not the same as Y up there */
} vs const Y = "SOME CONSTANT";
match x {
Y => /* here we match on x being the string "SOME CONSTANT" */
} |
Note: this is stacked on top of #645 How I plan to break the remaining stuff up:
|
What changes are proposed in this pull request?
This PR affects the following public APIs
How was this change tested?