TSDB: Simplify OOO Select by copying the head chunk#14396
TSDB: Simplify OOO Select by copying the head chunk#14396bboreham merged 3 commits intoprometheus:mainfrom
Conversation
Instead of carrying around extra fields in `Meta` structs which let us approximate what was in the chunk at the time, take a copy of the chunk. This simplifies lots of code, and lets us correct a couple of tests which were embedding the wrong answer. Signed-off-by: Bryan Boreham <[email protected]>
Signed-off-by: Bryan Boreham <[email protected]>
Was only used to constrain the OOO head chunk, which we now copy. Signed-off-by: Bryan Boreham <[email protected]>
| if headChunk && len(intervals) > 0 { | ||
| // Put the last interval in the head chunk | ||
| s1.ooo.oooHeadChunk = &oooHeadChunk{ | ||
| chunk: NewOOOChunk(), |
There was a problem hiding this comment.
This avoids crashing on a nil pointer, but is still unrealistic since an OOO chunk with zero samples should not be created in the real code.
| sample{t: minutes(25), f: float64(1)}, | ||
| sample{t: minutes(26), f: float64(0)}, | ||
| sample{t: minutes(30), f: float64(0)}, | ||
| sample{t: minutes(32), f: float64(1)}, // This sample was added after Series() but before Chunk() and its in between the lastmint and maxt so it should be kept |
There was a problem hiding this comment.
This behaviour was a bug - queries should not have results that vary depending on timing - so has been removed from the test.
| sample{t: minutes(25), f: float64(1)}, | ||
| sample{t: minutes(26), f: float64(0)}, | ||
| sample{t: minutes(30), f: float64(0)}, | ||
| sample{t: minutes(32), f: float64(1)}, // This sample was added after Series() but before Chunk() and its in between the lastmint and maxt so it should be kept |
There was a problem hiding this comment.
This behaviour was a bug - queries should not have results that vary depending on timing - so has been removed from the test.
| addChunk(c.minTime, c.maxTime, ref) | ||
| var xor chunkenc.Chunk | ||
| if len(c.chunk.samples) > 0 { // Empty samples happens in tests, at least. | ||
| xor, _ = c.chunk.ToXOR() // Ignoring error because it can't fail. |
There was a problem hiding this comment.
This could be ToXORBetweenTimestamps(oh.mint, oh.maxt), but I'll leave that change for a later PR.
| } | ||
| iterable = chk | ||
| } | ||
| mc.chunkIterables = append(mc.chunkIterables, iterable) |
There was a problem hiding this comment.
Future optimization: collapse the case where len(mc.chunkIterables) ends up as 1.
|
/prombench main |
|
(PromBench doesn't generate any OOO data, but we might see an improvement from the smaller |
jesusvazquez
left a comment
jesusvazquez
left a comment
There was a problem hiding this comment.
LGTM
This is a nice simplification of the code, thanks for going through it 👍
|
No substantial differences seen in PromBench. |
|
/prombench cancel |
|
Benchmark cancel is in progress. |
| // We can skip chunks that came in later than the last known OOOLastRef. | ||
| if chunkRef > meta.OOOLastRef { | ||
| break |
There was a problem hiding this comment.
Removing this without any replacement introduced a bug. The symptom is worst in compaction, where it can fail seeing overlapping chunks.
3 participants
Instead of carrying around extra fields in
Metastructs which let us approximate what was in the chunk at the time, take a copy of the chunk.This simplifies lots of code, and lets us correct a couple of tests which were embedding the wrong answer.
Fixes #11837