Exposed helper methods for reading index bytes. #492
Conversation
|
Circle CI does not look right... I guees it is some migration period to CircleCI and it is being set up, right? |
|
That was an internal method that should never have existed (it was a layering violation), nor are there any public methods that take offsets. I don't think this makes sense as part of the API.
Decoder should probably be private. |
|
Thanks for taking a look @brian-brazil ! (: I can see totally your concerns with limiting API, but we have IMO valid use case: Thanos as consumer of TSDB library wants to be able use helpers for decoding index bytes:
This is to be able to implement index cache, so storing part of index (version, symbol, values, posting index) in different place than rest (actual postings and series). Which allows us to use S3 API to get_range bytes we need from index. The truth is that TSDB format (including index format) is an API itself, so why not exposing useful helper methods for accessing it? I agree that on If you don't agree with reusability pattern, do you have any suggestion how in Thanos we should overcome lack of such helpers? |
|
Looking through it, I think the right way to do it would be to split the parsing logic from the API logic. This would make readSymbols and friends available, rather than changing the API. |
|
Sound Good, will give it a try. |
bwplotka
force-pushed
the
index-reader-symbols
branch
from
101e247
to
ae3eec0
Compare
|
PTAL @brian-brazil . Updated description with summary of changes. |
bwplotka
changed the title
| // after offset to hold the big endian encoded content length, followed by the contents and the expected | ||
| // checksum. | ||
| func newDecbufAt(bs ByteSlice, off int) decbuf { | ||
| if bs.Len() < off+4 { |
There was a problem hiding this comment.
straight copy of r.newDecbufAt just with ByteSlice passed by argument.
| // decbufUvarintAt returns a new decoding buffer. It expects the first bytes | ||
| // after offset to hold the uvarint-encoded buffers length, followed by the contents and the expected | ||
| // checksum. | ||
| func newDecbufUvarintAt(bs ByteSlice, off int) decbuf { |
bwplotka
force-pushed
the
index-reader-symbols
branch
5 times, most recently
from
ba20032
to
e944b59
Compare
index/index.go
Outdated
| res[s] = struct{}{} | ||
| } | ||
| for _, s := range r.symbolSlice { | ||
| for _, s := range r.symbolsV2 { | ||
| res[s] = struct{}{} | ||
| } | ||
| return res, nil | ||
| } | ||
|
|
||
| // SymbolTableSize returns the symbol table that is used to resolve symbol references. |
There was a problem hiding this comment.
This comment is out of date
index/index.go
Outdated
| labelIndicesTable uint64 | ||
| postings uint64 | ||
| postingsTable uint64 | ||
| // TOC represents index Table Of Content that states were each section of index starts. |
Changes: * Make `NewReader` method useful. It was impossible to use it, because closer was always nil. * ReadSymbols, TOC and ReadOffsetTable are not public functions (used by Thanos). * decbufXXX are now functions. * More verbose errors. * Removed unused crc32 field. * Some var name changes to make it more verbose: * symbols -> allocatedSymbols * symbolsSlice -> symbolsV1 * symbols -> symbolsV2 * * Pre-calculate symbolsTableSize. * Initialized symbols for Symbols() method with valid length. * Added test for Symbol method. * Made Decoder LookupSymbol method public. Kept Decode public as it is useful as helper from index package. Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
bwplotka
force-pushed
the
index-reader-symbols
branch
from
e944b59
to
9d4109d
Compare
|
Comments addressed. |
|
kind PTAL @brian-brazil |
|
Thanks! |
) Changes: * Make `NewReader` method useful. It was impossible to use it, because closer was always nil. * ReadSymbols, TOC and ReadOffsetTable are not public functions (used by Thanos). * decbufXXX are now functions. * More verbose errors. * Removed unused crc32 field. * Some var name changes to make it more verbose: * symbols -> allocatedSymbols * symbolsSlice -> symbolsV1 * symbols -> symbolsV2 * * Pre-calculate symbolsTableSize. * Initialized symbols for Symbols() method with valid length. * Added test for Symbol method. * Made Decoder LookupSymbol method public. Kept Decode public as it is useful as helper from index package. Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
Changes:
Signed-off-by: Bartek Plotka bwplotka@gmail.com
Signed-off-by: Bartek Plotka bwplotka@gmail.com