-
Notifications
You must be signed in to change notification settings - Fork 154
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
irmin-pack: add V5 version #2184
Conversation
Add `volume_num` to upper control file. Change version of volume control file to V5. Refactor checksum code to model valid and invalid payloads and allow checking checksums when upgrading.
e8dc4df
to
6ef41bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, this seems to make sense!
Just to make sure I have a good understanding of the invariants around volume_num
though, is it right to say that (in an ideal world when everything is implemented):
- if
volume_num > 0
, then the store definitely has a lower layer - if
volume_num = 0
after opening it, then the store definitely doesn't have a lower layer during this session (but one may be added at a later open), since even a newly created lower would havevolume_num = 1
after migration/creation
?
@Firobe thanks for the review! For definitional purposes, a lower layer contains 0 or more volumes, so I'm currently working on the code to load the lower and it is modeled as an option type on the file manager. If a lower root is configured, it is Here is how I am currently thinking about when to create the first volume in a lower (I think this agrees with what you said but open to discussion!):
All other increments of So, to loop around to where we began: I think in practice |
Thank you for the explanation! That was more or less what I had in mind: in practice yes, but not the primary signal. In what I'm working on I've used a placeholder optional field in file manager as well to track that, so that'll be easy to merge! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good! Regarding the version issue, I'm guessing that either the polymorphic variant don't play too well with repr
, or that the encoding changes depending on the number of constructors? (which would explain why status
has some reserved T1...T15
constructors) In any case, an integer version makes sense :)
…min-pack, irmin-pack-tools, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.7.0) CHANGES: ### Added - **irmin** - Add `Conf.pp` and `Conf.equal` to print and compare configuration values (mirage/irmin#2227, @samoht) - Add a `clear` optional arguments to all function that adds a new commit: `Commit.v`, `set`, `set_tree`, `remove`, `test_and_set`, `test_and_set_tree`, `test_set_and_get`, `test_set_and_get_tree`, `merge`, `merge_tree` and `with_tree`. This new argument allows to control whether the tree caches are cleared up after objects are exported to disk during the commit. (mirage/irmin#2225, @samoht) - **irmin-pack** - Add configuration option, `lower_root`, to specify a path for archiving data during a GC. (mirage/irmin#2177, @metanivek) - Add `is_split_allowed` to check if a store allows split. (mirage/irmin#2175, @metanivek) - Add `add_volume` to allow creating new empty volume in lower layer. (mirage/irmin#2188, @metanivek) - Add a `behaviour` function to the GC to check wether the GC will archive or delete data. (mirage/irmin#2190, @Firobe) - Add a migration on `open_rw` to move the data to the `lower_root` if the configuration was enabled (mirage/irmin#2205, @art-w) ### Changed - **irmin** - Expose type equality for `Schema.Info` to avoid defining the `info` function multiple times when using similar stores (mirage/irmin#2189, mirage/irmin#2193, @samoht) - **irmin-pack** - GC now changes its behaviour depending on the presence of a lower layer. (mirage/irmin#2190, @Firobe) - Split now raises an exception if it is not allowed. It is not allowed on stores that do not allow GC. (mirage/irmin#2175, @metanivek) - GC now supports stores imported V1/V2 stores, in presence of a lower layer only. (mirage/irmin#2190, @art-w, @Firobe) - Upgrade on-disk format to version 5. (mirage/irmin#2184, @metanivek) - Archive to lower volume does not copy orphaned commits. (mirage/irmin#2215, @art-w) ### Fixed - **irmin-pack** - Unhandled exceptions in GC worker process are now reported as a failure (mirage/irmin#2163, @metanivek) - Fix the silent mode for the integrity checks. (mirage/irmin#2179, @icristescu) - Fix file descriptor leak caused by `mmap`. (mirage/irmin#2232, @art-w)
This PR makes changes for the upper control file needed for the lower layer and bumps the irmin-pack version to V5.
volume_num
to upper control file.I ran into one small issue where I wanted to use
Version.t option
as a type in the control file (for tracking upgrades) but ran into an error in the tests that seems to be coming out of repr (or maybe theversion.ml
code -- my brief investigation didn't shed light on what was going on). To get around this, I changed the type toint option
and am usingVersion.to_int V4
when setting the value during an upgrade. It's not the most ideal but not the worst thing either. If anybody wants to try to dig further... 👼