refactor: add ReaderProvider to fetch registry from cache once per re… - #447
Open
forshev wants to merge 1 commit into
Open
refactor: add ReaderProvider to fetch registry from cache once per re…#447forshev wants to merge 1 commit into
forshev wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #447 +/- ##
==========================================
- Coverage 70.68% 70.52% -0.16%
==========================================
Files 233 234 +1
Lines 18399 18486 +87
==========================================
+ Hits 13005 13038 +33
- Misses 4416 4453 +37
- Partials 978 995 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Collaborator
|
@seqbenchbot up main search |
Collaborator
|
Oh-oh, @eguguchkin Something went wrong and I couldn't process your request. |
Collaborator
|
@seqbenchbot up main search-logbench |
Collaborator
|
Nice, @eguguchkin Your request was successfully served. Here is a list of helpful links: Have a great time! |
Collaborator
|
Nice, @eguguchkin The benchmark with identificator Show summary
Have a great time! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Collaborator
|
@seqbenchbot up main search-keyword-exact-match-warm |
Collaborator
|
Oh-oh, @eguguchkin Something went wrong and I couldn't process your request. |
Collaborator
|
@seqbenchbot up main search-keyword-exact-match-warm |
Collaborator
|
Nice, @eguguchkin Your request was successfully served. Here is a list of helpful links: Have a great time! |
Collaborator
|
@seqbenchbot down c98188f3 |
Collaborator
|
Nice, @eguguchkin The benchmark with identificator Show summary
Have a great time! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
eguguchkin
requested review from
cheb0,
eguguchkin and
moflotas
and removed request for
cheb0
June 29, 2026 10:59
forshev
force-pushed
the
444-optimize-registry-fetching
branch
3 times, most recently
from
July 13, 2026 12:38
3827c8a to
bfab9ab
Compare
forshev
force-pushed
the
444-optimize-registry-fetching
branch
from
July 27, 2026 09:12
bfab9ab to
3ea793d
Compare
eguguchkin
approved these changes
Jul 27, 2026
| } | ||
| } | ||
|
|
||
| func (r *ReaderProvider) GetReader() (IndexReader, error) { |
Collaborator
There was a problem hiding this comment.
nit: I think a more idiomatic name for this method would be just Reader?
| "github.com/ozontech/seq-db/cache" | ||
| ) | ||
|
|
||
| type ReaderProvider struct { |
Collaborator
There was a problem hiding this comment.
nit: Would it make sense to merge index_reader.go and index_provider.go into a single index_reader.go file?
| idReader = legacyReader | ||
| } else { | ||
| var err error | ||
| tokenReader, err = f.tokenReaderProvider.GetReader() |
Collaborator
There was a problem hiding this comment.
nit: This introduces a fair amount of error-handling boilerplate. What if we extracted this into a separate helper function?
func (f *Sealed) mustGetReader(p *storage.ReaderProvider) storage.IndexReader {
r, err := p.GetReader()
if err != nil {
logger.Fatal("error creating IndexReader", zap.String("fraction", f.BaseFileName), zap.Error(err))
}
return r
}We should also ensure that errors returned by provider include the filename to make it easier to pinpoint which specific index file caused the issue.
Something like this:
--- a/storage/reader_provider.go
+++ b/storage/reader_provider.go
@@ -48,10 +48,10 @@ func (r *ReaderProvider) readRegistry() ([]byte, error) {
n, err := r.limiter.ReadAt(r.reader, numBuf, 0)
if err != nil {
- return nil, fmt.Errorf("can't read disk registry, %s", err.Error())
+ return nil, fmt.Errorf("can't read disk registry from file %s: %w", r.readerName, err.Error())
}
if n == 0 {
- return nil, fmt.Errorf("can't read disk registry, n=0")
+ return nil, fmt.Errorf("can't read disk registry from file %s, n=0", r.readerName)
}
pos := binary.LittleEndian.Uint64(numBuf)
@@ -64,11 +64,11 @@ func (r *ReaderProvider) readRegistry() ([]byte, error) {
}
if uint64(n) != l {
- return nil, fmt.Errorf("can't read disk registry, read=%d, requested=%d", n, l)
+ return nil, fmt.Errorf("can't read disk registry, read=%d, requested=%d in file %s", n, l, r.readerName)
}
if len(buf)%IndexBlockHeaderSize != 0 {
- return nil, fmt.Errorf("wrong registry format")
+ return nil, fmt.Errorf("wrong registry format in file %s", r.readerName)
}
return buf, nilYes, having both the fraction name and the filename might be redundant since the filename already implies the fraction. However, I think we should aim for errors from fractions to always include a searchable fraction field to enable fast filtering in logs.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
…quest
Description
add ReaderProvider struct that creates IndexReader for each request
Fixes #444
If you have used LLM/AI assistance please provide model name and full prompt: