irmin-pack: clear LRU when reloading RO #2200
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
art-w
reviewed
Feb 27, 2023
art-w
approved these changes
Feb 27, 2023
In order to clear the LRU when a reload is called, this is a general refactor to move clearing of the LRU to happen when the prefix is reopened. This covers both a swap after a GC and when reload is called for a RO instance.
metanivek
force-pushed
the
ro_purge_lru
branch
from
8ec41b1
to
73e395b
Compare
metanivek
added a commit
to metanivek/opam-repository
that referenced
this pull request
Feb 28, 2023
…ils, irmin-test, irmin-pack, 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.5.2) CHANGES: ### Fixed - **irmin-pack** - Clear LRU when calling `reload` after a GC (mirage/irmin#2200, @metanivek)
metanivek
added a commit
to metanivek/opam-repository
that referenced
this pull request
Mar 7, 2023
…ils, irmin-test, irmin-pack, 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.6.1) CHANGES: ### Fixed - **irmin-pack** - Clear LRU when calling `reload` after a GC (mirage/irmin#2200, @metanivek)
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.
Currently, when a RO instance calls
reload, the LRU is not cleared. We have seen some issues in octez that we think is caused by this behavior. See #2171.When opening that issue, I originally made the assumption we did not have what we needed to "smartly" clear the LRU for RO instances, but I had an idea upon second consideration. During
reload, we only reopen the prefix if the generation changes so we can use that as our signal to clear the LRU. With some refactoring, that is what this PR does.In order to clear the LRU when
reloadis called, this is a general refactor to move clearing of the LRU to happen when the prefix is reopened. This covers both a swap after a GC and when reload is called for a RO instance.I added some new tests to verify the LRU has cleared for both a GC and a reload (and importantly it is not cleared if a GC has not occurred). They are a bit indirect by testing hits to the LRU, but get the job done. Open to other suggestions though!
This PR is to address #2171, but we shouldn't close that issue until this fix is merged to main. The reason this PR is targeted at 3.5 is that we will likely release this as a 3.5.2 release (for downstream octez usage), then we will forward port to
main. It is unclear if we will also need to do a 3.6.1 release.