compact: Verify for chunks outside of compacted time range. Added unit test for populateBlocs. #349
Conversation
|
why not open the PR against his branch |
|
Sure I am ok with it as long as we decide what to do in @BenoitKnecht PR (: |
|
time for a rebase and a review :) |
|
ping @bwplotka |
…t test for populateBlocs. Signed-off-by: Bartek Plotka <[email protected]>
bwplotka
force-pushed
the
issue347-extra-chunk-compaction
branch
from
52c79b2
to
ea2a31b
Compare
compact_test.go
Outdated
| @@ -401,3 +405,333 @@ type erringBReader struct{} | |||
| func (erringBReader) Index() (IndexReader, error) { return nil, errors.New("index") } | |||
| func (erringBReader) Chunks() (ChunkReader, error) { return nil, errors.New("chunks") } | |||
| func (erringBReader) Tombstones() (TombstoneReader, error) { return nil, errors.New("tombstones") } | |||
|
|
|||
| type mockedBReader struct { | |||
There was a problem hiding this comment.
lets call this just mockBReader , the same for mockIndexWriter
And lets put these in the corresponding files: mockBReader in block_test.go and mockIndexWriter in index_test.go
to make it easier to find when looking for mocks when writing new tests.
compact_test.go
Outdated
| func (nopChunkWriter) WriteChunks(chunks ...chunks.Meta) error { return nil } | ||
| func (nopChunkWriter) Close() error { return nil } | ||
|
|
||
| var populateBlocksCases = []struct { |
There was a problem hiding this comment.
why isn't this inside the TestCompaction_populateBlock defitnition?
compact_test.go
Outdated
| func (nopChunkWriter) Close() error { return nil } | ||
|
|
||
| var populateBlocksCases = []struct { | ||
| inputBlocks [][]seriesSamples |
There was a problem hiding this comment.
if this is a slice of seriesSamples why is it called inputBlocks ? later on it is used to created the chunk and index reader so maybe call it inputSeriesSamples or blockInput
the same for expectedBlock - expectedSeriesSamples or expectedBlockOutput
compact_test.go
Outdated
| var populateBlocksCases = []struct { | ||
| inputBlocks [][]seriesSamples | ||
| compactMinTime int64 | ||
| compactMaxTime int64 // by default it is math.MaxInt64 |
There was a problem hiding this comment.
🤖 golang robot : Capital letter and full stop.
btw I had to look down the code to understand what is that comment for so maybe change it to:
// When not defined the test runner sets a default of math.MaxInt64.
compact_test.go
Outdated
| expectedErr error | ||
| }{ | ||
| { | ||
| // Populate block from empty input should return error. |
There was a problem hiding this comment.
instead of comments why not add as a title?
Title: "Populate block from empty input should return error."
and than use it in t.Run(test.Title, this way if it fails it will also show the title of the test case that failed
compact_test.go
Outdated
| }, | ||
| compactMinTime: 0, | ||
| compactMaxTime: 20, | ||
| expectedErr: errors.New("found chunk with minTime: 10 maxTime: 30 outside of compacted minTime: 0 maxTime: 20"), |
There was a problem hiding this comment.
might be an overkill, but instead of error string matching we can define a custom error type and than match it.
expectedErr: tsdb.ErrChunkOutsider
_, ok := err.(tsdb.ErrChunkOutsider) or match the type directly
compact_test.go
Outdated
| chunks: [][]sample{{{t: 1}, {t: 9}}, {{t: 10}, {t: 19}}}, | ||
| }, | ||
| { | ||
| // no-chunk series should be dropped. |
There was a problem hiding this comment.
// Series without chunks should be dropped.
compact_test.go
Outdated
| inputBlocks: [][]seriesSamples{ | ||
| { | ||
| { | ||
| lset: map[string]string{"a": "issue347"}, |
There was a problem hiding this comment.
issue347 nice touch 😄 !
|
@bwplotka gentle reminder 😜 |
|
Yup, sorry, bit overloaded, will try to address it soon (current ETA is tomorrow) |
Moved mocks to dedicated file so they can be reused by other tests and few other small nits. Signed-off-by: Krasi Georgiev <[email protected]>
move mocks to dedicated file and some small nits
|
Thanks @krasi-georgiev for helping out! |
Rationales:
NOTE: This PR should be merged after: #348
Signed-off-by: Bartek Plotka [email protected]