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

envd: require explicit is_cc field in cluster size map #30432

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

benesch
Copy link
Contributor

@benesch benesch commented Nov 12, 2024

Rather than inferring the "cc"-ness of a cluster from its name. This will be important for self hosted Materialize, where customers will be able to install their own cluster size names. "cc"-ness defaults to true if not explicitly disabled, since "cc" clusters get modern behavior that we want all self-hosted clustrs to get.

Also, make it possible to disable using disk for a given size by setting the disk limit to 0, even for sizes marked as "cc", to support users that will be running Materialize on nodes without local disks.

Motivation

  • This PR refactors existing code.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

Sorry, something went wrong.

@def- def- force-pushed the is-cc branch 4 times, most recently from fcedad9 to 636264d Compare November 12, 2024 13:53
@def- def- marked this pull request as ready for review November 12, 2024 13:53
@def- def- requested review from a team as code owners November 12, 2024 13:53
@def- def- requested a review from ParkMyCar November 12, 2024 13:53
Copy link
Contributor

@def- def- left a comment

Choose a reason for hiding this comment

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

Tests are fine now, some didn't make any sense anymore after this change, so I removed the ones that didn't make sense to adapt.

Rather than inferring the "cc"-ness of a cluster from its name. This will
be important for self hosted Materialize, where customers will be able
to install their own cluster size names. "cc"-ness defaults to true if not
explicitly disabled, since "cc" clusters get modern behavior that we want
all self-hosted clustrs to get.

Also, make it possible to disable using disk for a given size by setting
the disk limit to 0, even for sizes marked as "cc", to support users
that will be running Materialize on nodes without local disks.
@antiguru antiguru mentioned this pull request Nov 13, 2024
5 tasks
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.

I think this looks good. Happy to see some simplification around how we decide to support disk for a replica.

@def- def- merged commit 3d1f7ab into MaterializeInc:main Nov 13, 2024
79 checks passed
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