-
Notifications
You must be signed in to change notification settings - Fork 480
Rollout strategy enum #33758
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
base: main
Are you sure you want to change the base?
Rollout strategy enum #33758
Conversation
c03e0b1
to
395b485
Compare
4260e6b
to
10b8db8
Compare
@def- I've manually tested upgrades for both variants, and I've run the nightly orchestratord tests. Is there anything else we need for this? We probably want to add this to the upgrade tests once those are ready, but I don't think that should block this PR. |
The terraform upgrade test, but that is currently WIP in another PR. So all good from QA side. |
10b8db8
to
aa2d6b5
Compare
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.
overall looks fine, other than the one minor issue
// consult with Materialize engineering to discuss your situation. | ||
ImmediatelyPromoteCausingDowntime, | ||
} | ||
impl Default for MaterializeRolloutStrategy { |
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.
you can use derive(Default)
and the #[default]
variant annotation instead of deriving this by hand
Deprecates and ignores the
inPlaceRollout
field of the Materialize CR, instead replacing it with a newrolloutStrategy
enum field.The enum has two variants:
WaitForRollout
(the default) andImmediatelyPromoteCausingDowntime
. Hopefully these new names will be more clear.We increment the the generation in both variants, so we avoid the weird behavior we saw with
inPlaceRollout
previously. Both variants follow almost the same code path, with the exceptions ofImmediatelyPromoteCausingDowntime
doing as its name implies, deleting the old generation first and immediately promoting the new generation.Motivation
Several customers have accidentally used
inPlaceRollout
, when they should not have. Even internally, this field is confusing, and it has lots of strange behavior caused by not incrementing the generation.Tips for reviewer
It's easier to review each commit individually.
Checklist
$T ⇔ Proto$T
mapping (possibly in a backwards-incompatible way), then it is tagged with aT-proto
label.