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

loosen version upgrade checks to allow for lts version upgrades #31079

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

doy-materialize
Copy link
Contributor

Motivation

lts releases are going to be on their own versioning scheme, and so we need to define the relationship between those versions and the existing weekly release versions to allow self-hosted users to upgrade and allow us to test the upgrade process between lts releases.

Tips for reviewer

i'm a bit nervous about this change because it is pretty critical to correctness - definitely open to better ideas around testing here (i wrote some more tests for the valid upgrade check but it's not super clear to me where tests should go for the catalog compatibility check).

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.

@doy-materialize doy-materialize requested review from a team as code owners January 16, 2025 23:07
@def-
Copy link
Contributor

def- commented Jan 17, 2025

i'm a bit nervous about this change because it is pretty critical to correctness - definitely open to better ideas around testing here

I was hoping we only need the test in a few weeks/months. The logic being that we don't do 0dt upgrades, and we mainly have to ensure that v0.130.0 is whitelisted as an allowed direct upgrade path to whatever the current main version is. We also have to make sure we keep migrations around for 6 months at least. And then test direct upgrades from it to main with the existing testing frameworks: legacy-upgrade and platform-checks. @jkosh44 Does this sound reasonable?

@jkosh44
Copy link
Contributor

jkosh44 commented Jan 17, 2025

we need to define the relationship between those versions and the existing weekly release versions to allow self-hosted users to upgrade and allow us to test the upgrade process between lts releases.

It would be helpful for me if we wrote down somewhere exactly what this relationship is and what guarantees we provide. It's not immediately clear to me from reading the code.

we mainly have to ensure that v0.130.0 is whitelisted as an allowed direct upgrade path to whatever the current main version is. We also have to make sure we keep migrations around for 6 months at least. ... @jkosh44 Does this sound reasonable?

Yes, I only want to add that we'd want to come up with a way to do this for all LTS versions going forward, not just v0.130.0.

Migrations should work fine, even if we skip over versions, but we also need @MaterializeInc/persist to weigh in on the forwards/backwards compatibility implications. Right now they only allow one version of forwards compatibility.

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.

3 participants