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

Remove Controller assertions and return errors instead #10704

Merged
merged 2 commits into from
Feb 16, 2022

Conversation

frankmcsherry
Copy link
Contributor

This PR changes the return type of Controller methods to present errors in cases the controller would panic due to dynamically verified constraints (e.g. all identifiers present, since frontiers are valid, etc).

All previous uses of these methods simply unwrap() their results, panicking just as they would before. However, there is now the possibility of some remediation whereas there would not be otherwise. The methods validate the transition before they take them, so that if an error is returned the command had no effect.

These validation steps could likely be broken out to independent methods to allow users (@mjibson, @sploiselle) to validate commands before launching them. We can do that in follow up work if it would be useful, but didn't want to overdo it here.

Motivation

  • This PR refactors existing code.

The coordinator was at the mercy of the Controller to determine what commands would work and which would not. Ideally this gets us closer to clearer communication and less guessing.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered.
  • This PR adds a release note for any user-facing behavior changes.

Copy link
Member

@antiguru antiguru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine

Copy link
Contributor

@sploiselle sploiselle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a dumb question, and sorry if I am behind everyone else's understanding: is instance equivalent to CLUSTER?

@frankmcsherry
Copy link
Contributor Author

@sploiselle instance is the rock-solid internal name for things that COMPUTE and STORAGE each spin up ("compute instance" and "storage instance"). No one could agree on what the user-facing name would be, so we agreed to use these internally and let the user names bind whenever folks manage to finish up their extended theses with supporting figures and graphs. You can use CLUSTER and be understood for sure, but I don't use it in case we change it to BANANA while I'm not looking.

@frankmcsherry frankmcsherry merged commit c9fd59c into MaterializeInc:main Feb 16, 2022
@frankmcsherry frankmcsherry deleted the controller_errors branch March 8, 2022 13:29
wangandi pushed a commit to wangandi/materialize that referenced this pull request Apr 11, 2022
…Inc#10704)

* Remove assertions and return errors instead

* Add Peek validation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants