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

[WIP] Move protocol validation to TableConfiguration #665

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

OussamaSaoudi
Copy link
Collaborator

What changes are proposed in this pull request?

This PR affects the following public APIs

How was this change tested?

@github-actions github-actions bot added the breaking-change Change that will require a version bump label Jan 29, 2025
@OussamaSaoudi
Copy link
Collaborator Author

OussamaSaoudi commented Jan 29, 2025

@scovich @nicklan @zachschuermann
Hey folks, this is a draft PR that I intend to break up, but I wanted some thoughts on the ideas I was working on. In particular, I want opinions on:

Error Types

The introduction of InvalidTableConfigurationError and SupportError. I want to introduce better errors in kernel. This is both for testability and for clearly communicating errors to users.

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:

  1. Can/Should I use with this style of error handling?
  2. Are you happy with the split of validity errors and support errors?

Debug Assertions

I want to start using more debug_assert s in the code, and want to hear your thoughts on how I've used them here.

try_new ensures that our table configuration is valid. I can assert that it continues to be valid in all the is_x_supported methods. Ex: [1].

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 Semantics

I 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 try_new checks both validity and support.

Here's where I used it for Transaction

Here's the one for Snapshot

TableProperty constants

aside: 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" */
}

@OussamaSaoudi
Copy link
Collaborator Author

OussamaSaoudi commented Jan 29, 2025

Note: this is stacked on top of #645

How I plan to break the remaining stuff up:

  1. Move protocol validation to table configuration
  2. Create invalidTableConfig and support error types
  3. add errors types to CDF error + all the refactoring involved with that
  4. Newtypes for transaction/tableConfig (if folks think this is a good direction)
  5. (possibly) encode table properties as an enum instead of the constant strings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Change that will require a version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant