irmin-pack: initial integration of lower into store api #2188
Conversation
metanivek
force-pushed
the
load_lower
branch
from
06ce783
to
817dfd7
Compare
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #2188 +/- ##
==========================================
+ Coverage 68.09% 68.13% +0.04%
==========================================
Files 135 135
Lines 16222 16294 +72
==========================================
+ Hits 11046 11102 +56
- Misses 5176 5192 +16
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Excellent work! It's nice seeing how the ideas are falling into place.
Interface changes look good, general logic also. Don't have a strong opinion on new error types, maybe somebody with more experience dealing with errors can provide their opinion.
Two minor comments.
|
@adatario just a few thoughts on errors... This is the first time I'm adding several new errors to our existing setup. For now, I'm trying to follow prior conventions, but I too am wondering what the best approach is. I think there is some balance between specificity and general, between an error for each case and To my knowledge, none of these errors leak outside of Here are two guiding principles when I'm thinking about errors:
|
metanivek
force-pushed
the
load_lower
branch
from
817dfd7
to
952f987
Compare
| @@ -786,6 +835,11 @@ struct | |||
| pl.chunk_start_idx pl.chunk_num]; | |||
| Control.set_payload t.control pl | |||
|
|
|||
| let add_volume t = | |||
| match t.lower with | |||
| | None -> Ok () (* TODO: throw exception? *) | |||
There was a problem hiding this comment.
I think it's ok not throw an exception and just continue irmin-pack'ing along.
I'd suggest adding a docstring in the File_manager interface specifying that if there is no lower setup than the add_volume call is a nop.
There was a problem hiding this comment.
Maybe also add a log message indicating that add_volume was called without a lower being conigured.
There was a problem hiding this comment.
I'd suggest adding a docstring in the
File_managerinterface specifying that if there is no lower setup than theadd_volumecall is a nop.
Correction: this should go in the store_intf.mli.
There was a problem hiding this comment.
Thanks for weighing in!
Usually I would agree that making this a no-op would be okay, but after thinking about this some more, I think it might be best to return an error to provide a clear signal to a client that something is likely wrong. It seems not ideal if a client calls this and thinks the operation succeeds but in reality they have not configured their store correctly. This would also be symmetrical with a call to GC failing when it is given the Archive action but no lower exists (not implemented yet but is the expected outcome).
I'll leave this as-is for the PR but follow up in a subsequent PR.
There was a problem hiding this comment.
I agree, especially since the function can already return an Error!
| (* TODO: if [volume_num] is 0, we need to migrate | ||
| (or return error if [no_migrate = true]) *) |
Thanks! That 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 does a few things to integrate the lower module into the higher level store code:
add_volumefunction to the store apiThere are some TODOs for subsequent PRs:
fresh, we need to recursively delete the lower root's volumesadd_volumeis called and no lower exists?(Note: this PR is based on #2184 so skip the first couple of commits if reviewing before it is merged)