Align series to 8/16 byte padding to increase addressable space #248
Conversation
index/index.go
Outdated
| // offset references in v1 are only 4 bytes large. | ||
| // Once we move to compressed/varint representations in those areas, this limitation | ||
| // can be lifted. | ||
| if w.pos > math.MaxUint32 { | ||
| if w.pos > 16*math.MaxUint32 { | ||
| return errors.Errorf("exceeding max size of 4GiB") |
index/index.go
Outdated
| SetSymbolTable(t map[uint32]string) | ||
| Postings(b []byte) (int, Postings, error) | ||
| Series(b []byte, lbls *labels.Labels, chks *[]chunks.Meta) error | ||
| } |
There was a problem hiding this comment.
We can probably skip making this an interface if v2 and thus DecoderV1 vs DecoderV2 is not necessary.
But generally, exported interfaces should not have any unexported methods.
index/index.go
Outdated
| @@ -250,6 +250,7 @@ func (w *Writer) writeMeta() error { | |||
| return w.write(w.buf1.get()) | |||
| } | |||
|
|
|||
| //AddSeries adds the series one at a time along with its chunks. | |||
block.go
Outdated
| @@ -151,6 +151,9 @@ type BlockMeta struct { | |||
|
|
|||
| // Information on compactions the block was created from. | |||
| Compaction BlockMetaCompaction `json:"compaction"` | |||
|
|
|||
| // Version of the index format | |||
| Version int `json:"version"` | |||
There was a problem hiding this comment.
Generally 👍 for making this explicit here. But I'm wondering whether we actually need to bump the format. The serialisation is certainly different with the padding, but it seems like DecoderV2 below doesn't do anything differently than DecoderV1.
There was a problem hiding this comment.
Yup, fair enough. I'll just rename it to Decoder instead.
There was a problem hiding this comment.
However, the main issue with having two different formats/versions was how to make them configurable. Currently, the value is being hardcoded as 1. However, picking it up from BlockMeta itself makes the test fail as there is no value specified in the test files and the default value is 0. How can that be fixed?
index/index.go
Outdated
| w.seriesOffsets[ref] = w.pos | ||
| w.addPadding(16) | ||
| w.seriesOffsets[ref] = w.pos / 16 | ||
| w.Version = 2 |
index/index.go
Outdated
| @@ -852,9 +866,13 @@ func (r *Reader) LabelIndices() ([][]string, error) { | |||
| return res, nil | |||
| } | |||
|
|
|||
| // Series the series with the given ID and writes its labels and chunks into lbls and chks. | |||
| // Reads the series with the given ID and writes its labels and chunks into lbls and chks. | |||
There was a problem hiding this comment.
In go the comments need to start with the function name. Series reads the.....
block.go
Outdated
| @@ -151,6 +151,9 @@ type BlockMeta struct { | |||
|
|
|||
| // Information on compactions the block was created from. | |||
| Compaction BlockMetaCompaction `json:"compaction"` | |||
|
|
|||
| // Version of the index format | |||
There was a problem hiding this comment.
Fullstop after comment.
index/index.go
Outdated
| @@ -563,27 +572,32 @@ func (b realByteSlice) Sub(start, end int) ByteSlice { | |||
| } | |||
|
|
|||
| // NewReader returns a new IndexReader on the given byte slice. | |||
| func NewReader(b ByteSlice) (*Reader, error) { | |||
| return newReader(b, nil) | |||
| func NewReader(b ByteSlice, v int) (*Reader, error) { | |||
There was a problem hiding this comment.
Name accepted argument version everywhere. Will become much clearer.
|
Also, the docs need to be updated about the format. |
index/index.go
Outdated
| return newReader(b, nil, version) | ||
| } | ||
|
|
||
| func NewReaderV1(b ByteSlice, c io.Closer, version int) (*Reader, error) { |
There was a problem hiding this comment.
Do we need this function with the above definition already doing the same thing? I know I was leaning towards a NewReaderV1 and NewReaderV2, but that doesn't look necessary right now.
There was a problem hiding this comment.
Also, if its V1 why is it taking a version ;)?
| } | ||
|
|
||
| func newReader(b ByteSlice, c io.Closer) (*Reader, error) { | ||
| func newReader(b ByteSlice, c io.Closer, version int) (*Reader, error) { | ||
| r := &Reader{ |
There was a problem hiding this comment.
Check if the version is 1 or 2 and return an error if its neither?
|
Nevermind that comment. Not sure where my head was. |
index/index.go
Outdated
| } | ||
|
|
||
| // NewFileReader returns a new index reader against the given index file. | ||
| func NewFileReader(path string) (*Reader, error) { | ||
| func NewFileReader(path string, version int) (*Reader, error) { | ||
| if version != 1 && version != 2 { |
There was a problem hiding this comment.
Can we move this check into newReader? Because there is another caller above for which we are not doing any checks :)
Fixes #238