Skip to content

Commit

Permalink
loosen version upgrade checks to allow for lts version upgrades
Browse files Browse the repository at this point in the history
  • Loading branch information
@doy-materialize @def-
doy-materialize authored and def- committed Jan 24, 2025
1 parent c260a8a commit eb48ec6
Show file tree
Hide file tree
Showing 2 changed files with 201 additions and 1 deletion.
56 changes: 55 additions & 1 deletion src/persist-client/src/cfg.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
];

/// The tunable knobs for persist.
///
/// Tuning inputs:
Expand Down Expand Up @@ -609,6 +614,10 @@ impl BlobKnobs for PersistConfig {
}
}

pub fn check_data_version(code_version: &Version, data_version: &Version) -> Result<(), String> {
check_data_version_with_lts_versions(code_version, data_version, LTS_VERSIONS)
}

// If persist gets some encoded ProtoState from the future (e.g. two versions of
// code are running simultaneously against the same shard), it might have a
// field that the current code doesn't know about. This would be silently
Expand All @@ -633,7 +642,51 @@ impl BlobKnobs for PersistConfig {
// data we read is going to be because we fetched it using a pointer stored in
// some persist state. If we can handle the state, we can handle the blobs it
// references, too.
pub fn check_data_version(code_version: &Version, data_version: &Version) -> Result<(), String> {
pub(crate) fn check_data_version_with_lts_versions(
code_version: &Version,
data_version: &Version,
lts_versions: &[Version],
) -> Result<(), String> {
// Allow upgrades specifically between consecutive LTS releases.
if data_version >= code_version {
for window in lts_versions.windows(2) {
if window[0]
== (Version {
patch: 0,
..code_version.clone()
})
&& data_version
<= (&Version {
patch: u64::MAX,
..window[1].clone()
})
{
eprintln!(
"{}-{} {} {}",
window[0], window[1], code_version, data_version,
);
return Ok(());
} else {
eprintln!(
"not {}-{} {} {}",
window[0], window[1], code_version, data_version,
);
}
}

if let Some(last) = lts_versions.last() {
if last
== (&Version {
patch: 0,
..code_version.clone()
})
{
eprintln!("{}- {} {}", last, code_version, data_version);
return Ok(());
}
}
}

// Allow one minor version of forward compatibility. We could avoid the
// clone with some nested comparisons of the semver fields, but this code
// isn't particularly performance sensitive and I find this impl easier to
Expand All @@ -649,6 +702,7 @@ pub fn check_data_version(code_version: &Version, data_version: &Version) -> Res
"{code_version} received persist state from the future {data_version}",
))
} else {
eprintln!("{} {}", code_version, data_version);
Ok(())
}
}
146 changes: 146 additions & 0 deletions src/persist-client/src/internal/encoding.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2059,6 +2059,152 @@ mod tests {
proptest!(|(state in any_state::<u64>(0..3))| testcase(state));
}

#[mz_ore::test]
fn check_data_versions_with_lts_versions() {
#[track_caller]
fn testcase(code: &str, data: &str, lts_versions: &[Version], expected: Result<(), ()>) {
let code = Version::parse(code).unwrap();
let data = Version::parse(data).unwrap();
let actual = cfg::check_data_version_with_lts_versions(&code, &data, lts_versions)
.map_err(|_| ());
assert_eq!(actual, expected);
}

let none = [];
let one = [Version::new(0, 130, 0)];
let two = [Version::new(0, 130, 0), Version::new(0, 140, 0)];
let three = [
Version::new(0, 130, 0),
Version::new(0, 140, 0),
Version::new(0, 150, 0),
];

testcase("0.130.0", "0.128.0", &none, Ok(()));
testcase("0.130.0", "0.129.0", &none, Ok(()));
testcase("0.130.0", "0.130.0", &none, Ok(()));
testcase("0.130.0", "0.130.1", &none, Ok(()));
testcase("0.130.1", "0.130.0", &none, Ok(()));
testcase("0.130.0", "0.131.0", &none, Ok(()));
testcase("0.130.0", "0.132.0", &none, Err(()));

testcase("0.129.0", "0.127.0", &none, Ok(()));
testcase("0.129.0", "0.128.0", &none, Ok(()));
testcase("0.129.0", "0.129.0", &none, Ok(()));
testcase("0.129.0", "0.129.1", &none, Ok(()));
testcase("0.129.1", "0.129.0", &none, Ok(()));
testcase("0.129.0", "0.130.0", &none, Ok(()));
testcase("0.129.0", "0.131.0", &none, Err(()));

testcase("0.130.0", "0.128.0", &one, Ok(()));
testcase("0.130.0", "0.129.0", &one, Ok(()));
testcase("0.130.0", "0.130.0", &one, Ok(()));
testcase("0.130.0", "0.130.1", &one, Ok(()));
testcase("0.130.1", "0.130.0", &one, Ok(()));
testcase("0.130.0", "0.131.0", &one, Ok(()));
testcase("0.130.0", "0.132.0", &one, Ok(()));

testcase("0.129.0", "0.127.0", &one, Ok(()));
testcase("0.129.0", "0.128.0", &one, Ok(()));
testcase("0.129.0", "0.129.0", &one, Ok(()));
testcase("0.129.0", "0.129.1", &one, Ok(()));
testcase("0.129.1", "0.129.0", &one, Ok(()));
testcase("0.129.0", "0.130.0", &one, Ok(()));
testcase("0.129.0", "0.131.0", &one, Err(()));

testcase("0.131.0", "0.129.0", &one, Ok(()));
testcase("0.131.0", "0.130.0", &one, Ok(()));
testcase("0.131.0", "0.131.0", &one, Ok(()));
testcase("0.131.0", "0.131.1", &one, Ok(()));
testcase("0.131.1", "0.131.0", &one, Ok(()));
testcase("0.131.0", "0.132.0", &one, Ok(()));
testcase("0.131.0", "0.133.0", &one, Err(()));

testcase("0.130.0", "0.128.0", &two, Ok(()));
testcase("0.130.0", "0.129.0", &two, Ok(()));
testcase("0.130.0", "0.130.0", &two, Ok(()));
testcase("0.130.0", "0.130.1", &two, Ok(()));
testcase("0.130.1", "0.130.0", &two, Ok(()));
testcase("0.130.0", "0.131.0", &two, Ok(()));
testcase("0.130.0", "0.132.0", &two, Ok(()));
testcase("0.130.0", "0.135.0", &two, Ok(()));
testcase("0.130.0", "0.138.0", &two, Ok(()));
testcase("0.130.0", "0.139.0", &two, Ok(()));
testcase("0.130.0", "0.140.0", &two, Ok(()));
testcase("0.130.9", "0.140.0", &two, Ok(()));
testcase("0.130.0", "0.140.1", &two, Ok(()));
testcase("0.130.3", "0.140.1", &two, Ok(()));
testcase("0.130.3", "0.140.9", &two, Ok(()));
testcase("0.130.0", "0.141.0", &two, Err(()));
testcase("0.129.0", "0.133.0", &two, Err(()));
testcase("0.129.0", "0.140.0", &two, Err(()));
testcase("0.131.0", "0.133.0", &two, Err(()));
testcase("0.131.0", "0.140.0", &two, Err(()));

testcase("0.130.0", "0.128.0", &three, Ok(()));
testcase("0.130.0", "0.129.0", &three, Ok(()));
testcase("0.130.0", "0.130.0", &three, Ok(()));
testcase("0.130.0", "0.130.1", &three, Ok(()));
testcase("0.130.1", "0.130.0", &three, Ok(()));
testcase("0.130.0", "0.131.0", &three, Ok(()));
testcase("0.130.0", "0.132.0", &three, Ok(()));
testcase("0.130.0", "0.135.0", &three, Ok(()));
testcase("0.130.0", "0.138.0", &three, Ok(()));
testcase("0.130.0", "0.139.0", &three, Ok(()));
testcase("0.130.0", "0.140.0", &three, Ok(()));
testcase("0.130.9", "0.140.0", &three, Ok(()));
testcase("0.130.0", "0.140.1", &three, Ok(()));
testcase("0.130.3", "0.140.1", &three, Ok(()));
testcase("0.130.3", "0.140.9", &three, Ok(()));
testcase("0.130.0", "0.141.0", &three, Err(()));
testcase("0.129.0", "0.133.0", &three, Err(()));
testcase("0.129.0", "0.140.0", &three, Err(()));
testcase("0.131.0", "0.133.0", &three, Err(()));
testcase("0.131.0", "0.140.0", &three, Err(()));
testcase("0.130.0", "0.150.0", &three, Err(()));

testcase("0.140.0", "0.138.0", &three, Ok(()));
testcase("0.140.0", "0.139.0", &three, Ok(()));
testcase("0.140.0", "0.140.0", &three, Ok(()));
testcase("0.140.0", "0.140.1", &three, Ok(()));
testcase("0.140.1", "0.140.0", &three, Ok(()));
testcase("0.140.0", "0.141.0", &three, Ok(()));
testcase("0.140.0", "0.142.0", &three, Ok(()));
testcase("0.140.0", "0.145.0", &three, Ok(()));
testcase("0.140.0", "0.148.0", &three, Ok(()));
testcase("0.140.0", "0.149.0", &three, Ok(()));
testcase("0.140.0", "0.150.0", &three, Ok(()));
testcase("0.140.9", "0.150.0", &three, Ok(()));
testcase("0.140.0", "0.150.1", &three, Ok(()));
testcase("0.140.3", "0.150.1", &three, Ok(()));
testcase("0.140.3", "0.150.9", &three, Ok(()));
testcase("0.140.0", "0.151.0", &three, Err(()));
testcase("0.139.0", "0.143.0", &three, Err(()));
testcase("0.139.0", "0.150.0", &three, Err(()));
testcase("0.141.0", "0.143.0", &three, Err(()));
testcase("0.141.0", "0.150.0", &three, Err(()));

testcase("0.150.0", "0.148.0", &three, Ok(()));
testcase("0.150.0", "0.149.0", &three, Ok(()));
testcase("0.150.0", "0.150.0", &three, Ok(()));
testcase("0.150.0", "0.150.1", &three, Ok(()));
testcase("0.150.1", "0.150.0", &three, Ok(()));
testcase("0.150.0", "0.151.0", &three, Ok(()));
testcase("0.150.0", "0.152.0", &three, Ok(()));
testcase("0.150.0", "0.155.0", &three, Ok(()));
testcase("0.150.0", "0.158.0", &three, Ok(()));
testcase("0.150.0", "0.159.0", &three, Ok(()));
testcase("0.150.0", "0.160.0", &three, Ok(()));
testcase("0.150.9", "0.160.0", &three, Ok(()));
testcase("0.150.0", "0.160.1", &three, Ok(()));
testcase("0.150.3", "0.160.1", &three, Ok(()));
testcase("0.150.3", "0.160.9", &three, Ok(()));
testcase("0.150.0", "0.161.0", &three, Ok(()));
testcase("0.149.0", "0.153.0", &three, Err(()));
testcase("0.149.0", "0.160.0", &three, Err(()));
testcase("0.151.0", "0.153.0", &three, Err(()));
testcase("0.151.0", "0.160.0", &three, Err(()));
}

#[mz_ore::test]
fn check_data_versions() {
#[track_caller]
Expand Down

0 comments on commit eb48ec6

Please sign in to comment.