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

fix(file source): fix regression in fingerprint calculations #8187

Closed
wants to merge 1 commit into from

Conversation

jszwedko
Copy link
Member

@jszwedko jszwedko commented Jul 8, 2021

0.14.0 included an upgrade to the crc crate, with associated updates
to use the new API, however it appears to calculate different checksums.

I put a comment here asking about how to calculate an equivalent
checksum using the new API:
mrhooray/crc-rs#62 (comment)

This also adds an alias to ensure that checkpoints written by 0.14.0
will be able to be read by 0.15.0 when it is released.

Fixes: #8182

Signed-off-by: Jesse Szwedko [email protected]

0.14.0 included an upgrade to the `crc` crate, with associated updates
to use the new API, however it appears to calculate different checksums.

I put a comment here asking about how to calculate an equivalent
checksum using the new API:
mrhooray/crc-rs#62 (comment)

This also adds an alias to ensure that checkpoints written by 0.14.0
will be able to be read by 0.15.0 when it is released.

Fixes: #8182

Signed-off-by: Jesse Szwedko <[email protected]>
@jszwedko jszwedko requested review from lukesteensen and a team July 8, 2021 20:56
Copy link
Member

@lukesteensen lukesteensen left a comment

Choose a reason for hiding this comment

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

The only issue I see with this is that it's going to be incompatible with checksums from 0.14.0. I'm not sure how best to resolve that, since calculating and comparing both seems like it'd be a big pain.

I would also add a simple test that compares the full serialized JSON of the checkpoints file to a fixed string, so that we'll get a failure and see a diff anytime it changes. Something like that should catch both the crc and the variant rename issues.

@jszwedko
Copy link
Member Author

jszwedko commented Jul 8, 2021

The only issue I see with this is that it's going to be incompatible with checksums from 0.14.0. I'm not sure how best to resolve that, since calculating and comparing both seems like it'd be a big pain.

I would also add a simple test that compares the full serialized JSON of the checkpoints file to a fixed string, so that we'll get a failure and see a diff anytime it changes. Something like that should catch both the crc and the variant rename issues.

Blah, yeah, that's true, it will be incompatible with the changes in 0.14.0 now. I don't think there is a way to differentiate the checksums written by 0.13.1 vs. 0.14.0. We could try to calculate both checksums but it would be a pain as you note. Maybe we should just accept the regression, document it on the release page for 0.14.0, and include the tests to guard against it in the future.

@lukesteensen
Copy link
Member

One potential option would be to do something similar to the existing maybe_upgrade method for legacy checkpoints. Basically make one pass at startup where we calculate both and upgrade any old ones to new ones.

@jszwedko
Copy link
Member Author

jszwedko commented Jul 9, 2021

Replaced by #8225

@jszwedko jszwedko closed this Jul 9, 2021
@jszwedko jszwedko deleted the revert-crc-update branch July 9, 2021 23:53
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.

File checkpoints are incompatible between 0.13.1 and 0.14.0
2 participants