Properly initialize head time #339
Conversation
| if atomic.CompareAndSwapInt64(&h.minTime, lt, mint) { | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
why are these in a loops and not if else blocks?
There was a problem hiding this comment.
Because CompareAndSwap can fail and we have to retry it then.
There was a problem hiding this comment.
thanks! I always thought these are blocking until they succeed.
| @@ -183,7 +183,7 @@ func NewHead(r prometheus.Registerer, l log.Logger, wal *wal.WAL, chunkRange int | |||
| wal: wal, | |||
| logger: l, | |||
| chunkRange: chunkRange, | |||
| minTime: math.MinInt64, | |||
| minTime: math.MaxInt64, | |||
| maxTime: math.MinInt64, | |||
There was a problem hiding this comment.
When I read the code later down I understand why minTime=MaxInt64 and maxTime=MinInt64 , but do you think some comments here might make it less confusing why these are reversed?
| @@ -200,17 +200,17 @@ func NewHead(r prometheus.Registerer, l log.Logger, wal *wal.WAL, chunkRange int | |||
| // them on to other workers. | |||
| // Samples before the mint timestamp are discarded. | |||
There was a problem hiding this comment.
// Samples before the minValidTime timestamp are discarded.
| @@ -462,10 +482,7 @@ func (h *Head) Truncate(mint int64) error { | |||
| // for a compltely fresh head with an empty WAL. | |||
There was a problem hiding this comment.
compltely -> completely
| mint, maxt int64 | ||
| head *Head | ||
| minValidTime int64 // No samples below this timestamp are allowed. | ||
| mint, maxt int64 | ||
|
|
There was a problem hiding this comment.
Maybe some comments why we need separate mint and minValidTime ?
db_test.go
Outdated
| @@ -1011,3 +1013,109 @@ func TestOverlappingBlocksDetectsAllOverlaps(t *testing.T) { | |||
| {Min: 8, Max: 9}: {nc1[8], nc1[9]}, // 7-10, 8-9 | |||
| }, OverlappingBlocks(nc1)) | |||
| } | |||
|
|
|||
| func TestInitializeHeadTimestamp(t *testing.T) { | |||
There was a problem hiding this comment.
shouldn't this test be in head_test.go?
There was a problem hiding this comment.
actually there is already a head test where you can just add few lines similar to what I did in the other PR
https://github.com/prometheus/tsdb/pull/334/files#diff-0dc7b53c06c14aba584e5b4d6986fa07R96
There was a problem hiding this comment.
The test covers cases with existing blocks, which is beyond the scope of Head. DB has to startup correctly and make some calls of Head's methods in the right order and with the right arguments. That's nothing we can test with an isolated Head.
| defer os.RemoveAll(dir) | ||
|
|
||
| db, err := Open(dir, nil, nil, nil) | ||
| testutil.Ok(t, err) |
There was a problem hiding this comment.
can't we use the openTestDB helper?
There was a problem hiding this comment.
looking it again it is the same number of lines so all the same.
This fixes various issues when initializing the head time range under different starting conditions. Signed-off-by: Fabian Reinartz <[email protected]>
|
Rebased and resolved conflicts. |
|
👍 |
This fixes various issues when initializing the head time range
under different starting conditions.
Depends on #332
@krasi-georgiev as promised
@jkohen FYI as it depends on the new WAL stuff, but probably not too interesting for you