irmin-pack: fix GC when target commit is in lower #2242
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 #2242 +/- ##
==========================================
+ Coverage 67.93% 67.94% +0.01%
==========================================
Files 136 136
Lines 16603 16622 +19
==========================================
+ Hits 11279 11294 +15
- Misses 5324 5328 +4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
We want to create a prefix/mapping in the upper in this scenario to support the migration of existing stores so that they can continue to call GC for data in the lower while getting the benefit of the sparse file in the upper.
Since any commit can be the target of a GC when a store's behavior is archive, we need to guard against calling GC on commits prior to the most recent successful GC. When store behavior is delete, this is automatically enforced since older commits are inaccessible.
art-w
approved these changes
May 3, 2023
metanivek
added a commit
to metanivek/opam-repository
that referenced
this pull request
May 3, 2023
…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.1) CHANGES: ### Fixed - **irmin-pack** - Fix issue when migrating v2 stores to use lower layer (@metanivek, mirage/irmin#2241) - Fix issue when calling GC for a commit in the lower after migration (@metanivek, mirage/irmin#2242)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 astringto keep from adding another error to our already long list of errors. It also finds an internal use for thelatest_target_gc_offsetfield. 🎉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.