irmin-pack: appending into the sparse file #2192
Conversation
There was a problem hiding this comment.
These changes look great! Lots of improvements. 🎉 Left some small comments for consideration.
For the commit history, I think it would be nice to clean up some of the (slightly thrashy) changes of the append/write modifications in sparse file (looking at commit 2, 3, and 4). I could see either all of them being squashed into the final commit (essentially the new sparse file api + gc integration) or one commit before the GC integration that completely introduces the new API for sparse file (namely, Wo and Ao modules).
src/irmin-pack/unix/sparse_file.ml
Outdated
| let open_ao ~mapping ~data = | ||
| let open Result_syntax in | ||
| let ao_open path = | ||
| (* TODO: the expected [end_poff] must be obtained from the control file *) |
There was a problem hiding this comment.
Could you add this to the function now? IIUC, it would just be end_poff:Int63.zero for current GC usage when creating the new sparse file and would get this code ready for the future when appending to a volume's sparse file. 🔮
There was a problem hiding this comment.
Yes, done! It also checks that the data file has the right size (from the information available in the mapping file)
There was a problem hiding this comment.
Ah, I see that yep! If we keep this pattern, do you think we should remove data_poff from the volume control file and just let mapping_poff drive it all?
There was a problem hiding this comment.
Yes the volume control file's data_end_poff is going to be redundant (and I think it's more important to check that the mapping has the right expectation). I'm not sure if this PR is the right moment to remove it, as it's going to trigger errors in other pending PRs?
There was a problem hiding this comment.
I think it can be done in a follow up PR. Just wanted to confirm we're on the same page about removing it. 👍
art-w
force-pushed
the
sparse-ao
branch
2 times, most recently
from
5832570
to
2042460
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 #2192 +/- ##
==========================================
- Coverage 68.14% 68.00% -0.15%
==========================================
Files 135 135
Lines 16313 16301 -12
==========================================
- Hits 11117 11086 -31
- Misses 5196 5215 +19
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
To allow updating the lower volumes :)
(This was a good opportunity to refactor the GC to use the new append functionality, and hence test that it works!)