feat(share/eds): Rework accessor cache #2612
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
walldiss
requested review from
renaynay,
Wondertan,
vgonkivs and
distractedm1nd
as code owners
- update metrics - fix tests
# Conflicts: # share/eds/blockstore.go # share/eds/store.go
- fix merge conflicts
walldiss
force-pushed
the
split-accessor-cache
branch
from
c870854
to
c6e7693
Compare
|
Needs rework, will update soon. |
walldiss
force-pushed
the
split-accessor-cache
branch
from
458665d
to
8117867
Compare
walldiss
force-pushed
the
split-accessor-cache
branch
from
8117867
to
0a2354f
Compare
walldiss
changed the title
# Conflicts: # share/eds/metrics.go
walldiss
changed the title
# Conflicts: # share/eds/accessor_cache.go # share/eds/metrics.go # share/eds/store.go
walldiss
force-pushed
the
split-accessor-cache
branch
from
4a8e1bf
to
c7308c5
Compare
walldiss
force-pushed
the
split-accessor-cache
branch
from
6c06553
to
0dfb514
Compare
walldiss
force-pushed
the
split-accessor-cache
branch
from
0dfb514
to
535a418
Compare
walldiss
force-pushed
the
split-accessor-cache
branch
from
6c46937
to
cee7338
Compare
Codecov Report
@@ Coverage Diff @@
## main #2612 +/- ##
==========================================
+ Coverage 51.37% 51.56% +0.18%
==========================================
Files 159 162 +3
Lines 10699 10755 +56
==========================================
+ Hits 5497 5546 +49
- Misses 4723 4726 +3
- Partials 479 483 +4
|
distractedm1nd
requested changes
Sep 6, 2023
distractedm1nd
requested changes
Sep 6, 2023
derrandz
reviewed
Sep 8, 2023
walldiss
force-pushed
the
split-accessor-cache
branch
from
c92b0ab
to
795735a
Compare
Wondertan
reviewed
Sep 13, 2023
- track close errors in metrics - move BlockstoreCloser and readCloser to utils - add comment for sleep in test
Wondertan
reviewed
Sep 14, 2023
- make errCacheMiss private - fix go.mod
Wondertan
approved these changes
Sep 14, 2023
distractedm1nd
approved these changes
Sep 15, 2023
6 tasks
vgonkivs
pushed a commit
to vgonkivs/celestia-node
that referenced
this pull request
Oct 5, 2023
**Motivation** Previously, all accessors went into cache. They were closed only when they were evicted from it and there was no way to manually close one. Now, when we need to cache accessors only in some cases and in some not to, this approach will not work. Enabling control of accessor close required major cache rework. New cache features: - Old cache didn't allow to close and release of an accessor. Now, Instead of returning just a reader/blockstore, cache returns object that composed of builder and `io.Closer` (Accessor interface). This allows caller to have an ability to close and release accessor once it is done with reading from it. - Items stored in cache also provide io.Closer`, but it is noop. Lifecycle of cached items is controlled by cache itself. For now objects are released right after they are evicted from cache, but it could be changed to smarter release based on current readers count. Will be implemented as separate issue. - Lazy blockstore builder. Old cache created blockstore every time accessor was acquired, even when only reader was used and blockstore was not needed. Creating blockstore is costly operation that involves building index, so this was reworked to create blockstore, only when it is requested and store it for subsequent requests. - adds cache unit tests. - adds metrics for cache methods to track cache hits/miss. - general cleanup of unused methods/logic. It will be easier for reviewer to start from `store.go` and `blockstore.go` and then go into new `cache` pkg
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.
Motivation
Previously, all accessors went into cache. They were closed only when they were evicted from it and there was no way to manually close one. Now, when we need to cache accessors only in some cases and in some not to, this approach will not work. Enabling control of accessor close required major cache rework.
New cache features:
io.Closer(Accessor interface). This allows caller to have an ability to close and release accessor once it is done with reading from it.It will be easier for reviewer to start from
store.goandblockstore.goand then go into newcachepkg