mirage / irmin

Irmin is a distributed database that follows the same design principles as Git
https://irmin.org
ISC License
1.83k stars 154 forks source link

irmin-pack: fix GC when target commit is in lower #2242

Closed metanivek closed 1 year ago

metanivek commented 1 year ago

After an existing store migrates to the lower layer, we want a GC that targets a commit in the lower layer to still run and create a sparse file (prefix + mapping) in the upper so that reads for new data do not need to go to the lower.

Before this PR, this kind of GC almost worked. The issue is that it would fail when attempting to swap the volume control file since a new temporary one was not created. This PR adds a test for this scenario and enables a previously written test that also failed with this bug.

(@Firobe tagged you for review since you wrote the original test if you wanted to double-check my changes that enable it)

Update1: I just added a commit that now checks that a requested commit for GC is strictly newer than the previous GC commit. I reused an existing error (Gc_disallowed) by adding a string to keep from adding another error to our already long list of errors. It also finds an internal use for the latest_target_gc_offset field. :tada:

Update2: There was another issue hiding if you called GC on a commit older than the latest commit in the lower. Thanks @art-w for spotting! This is now fixed. The root issue was that previous code assumed the offset of the GC commit should always be the new start offset of the suffix. But, we don't necessarily want that when calling GC on commits in the lower after a migration.

codecov-commenter commented 1 year ago

Codecov Report

Merging #2242 (08722c8) into main (4c38a4b) will increase coverage by 0.01%. The diff coverage is 80.00%.

:exclamation: Current head 08722c8 differs from pull request most recent head 2bec6bf. Consider uploading reports for the commit 2bec6bf to get more accurate results

:mega: 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    #2242      +/-   ##
==========================================
+ Coverage   67.93%   67.94%   +0.01%     
==========================================
  Files         136      136              
  Lines       16603    16622      +19     
==========================================
+ Hits        11279    11294      +15     
- Misses       5324     5328       +4     
Impacted Files Coverage Δ
src/irmin-pack/unix/errors.ml 28.88% <ø> (ø)
src/irmin-pack/unix/gc_worker.ml 3.24% <0.00%> (ø)
src/irmin-pack/unix/io_errors.ml 58.33% <ø> (ø)
src/irmin-pack/unix/gc.ml 72.64% <75.00%> (-0.77%) :arrow_down:
src/irmin-pack/unix/lower.ml 61.98% <100.00%> (+0.91%) :arrow_up:
src/irmin-pack/unix/store.ml 65.78% <100.00%> (+0.34%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more