[WIP] Add Chunks method to Series interface. #659
Conversation
Signed-off-by: Tom Wilkie <[email protected]>
tomwilkie
changed the title
| } | ||
|
|
||
| func (s *chainedSeries) Chunks() []chunks.Meta { | ||
| var chunks []chunks.Meta |
There was a problem hiding this comment.
Size of the slice can be preallocated to len(s.series)
| } | ||
|
|
||
| func (s *verticalChainedSeries) Chunks() []chunks.Meta { | ||
| var chunks []chunks.Meta |
There was a problem hiding this comment.
Size of the slice can be preallocated to len(s.series)
| // Labels returns the complete set of labels identifying the series. | ||
| Labels() labels.Labels | ||
|
|
||
| // Iterator returns a new iterator of the data of the series. |
There was a problem hiding this comment.
Worth to mention that we iterate here over raw samples? SeriesIterator does not tell that.
bwplotka
left a comment
bwplotka
left a comment
There was a problem hiding this comment.
Thanks @tomwilkie ! Super happy this is happening, but I think we might want ChunkIterator instead? 🤔
| } | ||
|
|
||
| func (s *chunkSeries) Chunks() []chunks.Meta { | ||
| return s.chunks |
There was a problem hiding this comment.
Is that a copy of chunks as comment suggests? I think it is exactly the same underlying array passed around.
| Iterator() SeriesIterator | ||
|
|
||
| // Chunks returns a copy of the compressed chunks that make up this series. | ||
| Chunks() []chunks.Meta |
There was a problem hiding this comment.
I think Chunks is breaking the consistency here a bit, becaue we have Iterator above so maybe ChunksIterator would be more appropriate? and have ChunkIterator iterator that iterates over chunks.
I know iterator interface is sometimes bit controversial (e.g very hard to read and debug), but especially for remote read use case it might be actually more valuable to have it in iterator. This is to otentially avoid allocating the whole slice of chunks within Series interface implementation once we compose the frame. For Cortex use case, I am not sure as you just mentioned: This is to enable Cortex to use TSDB What do you think?
of the compressed chunks
Does it mean we guarantee ALL implementations to return chunks with chunks.Meta.Bytes and not only Ref? I don't think we can guarantee that so we might want to add or ref to chunk in chunk file
There was a problem hiding this comment.
Proposed iterators here: #665 (: Let me know if this makes sense.
There was a problem hiding this comment.
I think the iterator approach is better.
|
Proposed alternative: #665 |
|
Closing for the alternative. |
Signed-off-by: Tom Wilkie [email protected]
This is to enable Cortex to use TSDB, and so the remote_read API doesn't need to re-encode chunks.
/cc @bwplotka.
TODO