loosen version upgrade checks to allow for lts version upgrades #31079
Conversation
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? |
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.
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. |
doy-materialize
force-pushed
the
push-yzkkkoprlllq
branch
from
6fda36a to
7ac77f2
Compare
| @@ -36,6 +36,11 @@ use crate::operators::STORAGE_SOURCE_DECODE_FUEL; | |||
| use crate::project::OPTIMIZE_IGNORED_DATA_DECODE; | |||
| use crate::read::READER_LEASE_DURATION; | |||
|
|
|||
| const LTS_VERSIONS: &[Version] = &[ | |||
| // 25.1 | |||
| Version::new(0, 130, 0), | |||
There was a problem hiding this comment.
Does the patch number matter here? (It shouldn't because we might have to bump it a few times)
There was a problem hiding this comment.
no, patch version shouldn't matter.
def-
force-pushed
the
push-yzkkkoprlllq
branch
from
7ac77f2 to
763cdfe
Compare
def-
force-pushed
the
push-yzkkkoprlllq
branch
from
763cdfe to
d19eec2
Compare
There was a problem hiding this comment.
E2E tests added in a new commit, nightly run should be green: https://buildkite.com/materialize/nightly/builds/10954 (Edit: green)
def-
force-pushed
the
push-yzkkkoprlllq
branch
2 times, most recently
from
9c6b6c4 to
eb3fc53
Compare
doy-materialize
force-pushed
the
push-yzkkkoprlllq
branch
from
eb3fc53 to
b25e51f
Compare
|
@bkirwi: how's this? |
doy-materialize
added
the
lts-backport
There was a problem hiding this comment.
Thanks for the changes! I think this now behaves like we'd expect.
This should be okay to merge whenever, since we're past 0.130.x on main, but we should make sure we only backport it once we've landed all the other forward-compatible changes we need to backport...
|
yeah, i'm going to merge it now so that we can start getting the upgrade tests running regularly, but yeah, we should start coordinating on the changes that will need to be backported soon |
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
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.