Skip to content

Conversation

@jorisdral
Copy link
Collaborator

No description provided.

@jorisdral jorisdral self-assigned this Oct 28, 2024
@jorisdral jorisdral force-pushed the jdral/snapshot-encode-decode branch 2 times, most recently from a8acd3b to 1d182b6 Compare October 28, 2024 21:10
@jorisdral jorisdral force-pushed the jdral/snapshot-metadata branch from 642a493 to 6e5b3ae Compare November 4, 2024 15:47
Copy link
Collaborator

@mheinzel mheinzel left a comment

Choose a reason for hiding this comment

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

Looks reasonable, but maybe @dcoutts should also take a quick look.

@jorisdral jorisdral force-pushed the jdral/snapshot-encode-decode branch from 1d182b6 to cf0356d Compare November 4, 2024 17:13
@jorisdral jorisdral force-pushed the jdral/snapshot-metadata branch from 6e5b3ae to a84ca60 Compare November 4, 2024 21:00
@jorisdral jorisdral force-pushed the jdral/snapshot-encode-decode branch from cf0356d to ad4f53d Compare November 4, 2024 22:38
@jorisdral jorisdral force-pushed the jdral/snapshot-metadata branch from e549686 to 9a9adc5 Compare November 5, 2024 10:41
@jorisdral jorisdral force-pushed the jdral/snapshot-encode-decode branch 2 times, most recently from f599dbf to c6cb049 Compare November 5, 2024 11:34
@dcoutts
Copy link
Collaborator

dcoutts commented Nov 5, 2024

The explanation of major/minor has gone after the rebase. I was drafting an alternative.

I think the precision doesn't really aid comprehension here. How about:

-- A snapshot format version @x.y@ has two components: a major version number
-- @x@ and a minor version number @y@. The major version number is used to
-- signal breaking changes and the minor version number is used to signal
-- non-breaking changes.
--
-- A single release of the library may support several major snapshot format
-- versions, and thereby provide backwards compatibility. Support for old
-- major versions is not guaranteed indefinitely however. Forwards
-- compatibility is not provided at all: snapshots with a later major version
-- than the current version for the library release will always fail.
--
-- The minor version has no influence on whether a snapshot can be loaded or not:
-- minor version format changes are always compatible. They may however have an
-- effect, since later minor versions may have more information, that
-- corresponding releases can decode. These effects will never affect functional
-- correctness.

@jorisdral
Copy link
Collaborator Author

jorisdral commented Nov 5, 2024

@dcoutts done

Base automatically changed from jdral/snapshot-metadata to main November 5, 2024 11:48
We also add roundtrip tests for each type in the hierarchy of snapshot metadata.
@jorisdral jorisdral force-pushed the jdral/snapshot-encode-decode branch from 076dfaa to 038cad3 Compare November 5, 2024 19:12
Copy link
Collaborator

@mheinzel mheinzel left a comment

Choose a reason for hiding this comment

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

I think that's better 👍

-- would lead to errors when loading an older snapshot, then the major version
-- should be increased as well.
data SnapshotVersion = V0_0
-- A snapshot format version has one component: a version number @x@. A single
Copy link
Collaborator

Choose a reason for hiding this comment

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

A simpler way of saying this would be "a snapshot format version is a number", maybe also mentioning they're consecutive and increasing?

Comment on lines 74 to 75
-- is not provided at all: snapshots with a later major version than the current
-- version for the library release will always fail.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
-- is not provided at all: snapshots with a later major version than the current
-- version for the library release will always fail.
-- is not provided at all: snapshots with a later version than the current
-- version for the library release will always fail.

Copy link
Collaborator

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

Looking good!

… version number.

We now have a single version number. Backwards compatibility is guaranteed for
at least 1 version, and forwards compatibility is not guaranteed at all.
@jorisdral jorisdral force-pushed the jdral/snapshot-encode-decode branch from 038cad3 to 5ba267d Compare November 8, 2024 12:53
@jorisdral jorisdral enabled auto-merge November 8, 2024 12:53
@jorisdral jorisdral added this pull request to the merge queue Nov 8, 2024
Merged via the queue into main with commit 14950a7 Nov 8, 2024
24 checks passed
@jorisdral jorisdral deleted the jdral/snapshot-encode-decode branch November 8, 2024 14:16
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.

4 participants