Enhanced WAL replay for duplicate series record#7438
Enhanced WAL replay for duplicate series record#7438codesome merged 1 commit intoprometheus:mainfrom
Conversation
|
I have simplified it further now compared to what it was in #7229 on a simple assertion mentioned in the PR description. Earlier I was comparing the m-mapped chunks of new series id with the old series id, which made it complex. Now we just keep m-mapped chunks for the series id in the latest record. |
a09f119 to
52f66ed
Compare
|
This now has conflicts |
|
I think I will be taking care of this in #7229, but will keep this open till then in case I don't do it there. So, no need to review this PR right now. (I will be getting to snapshotting chunks in a few weeks, caught up with some other stuff) |
|
I take back my above comment. Let's do it in this PR itself to keep the review of snapshot PR simpler. I have rebased this and is ready for another review. |
|
Hello @bwplotka Are you interested in reviewing this? Thanks! |
bwplotka
left a comment
There was a problem hiding this comment.
Some pair review comments (:
|
With some more ordering assumptions that we can make, the code is much simpler now and easy to understand. I have push the changes. |
|
But it has some races :( I will fix them |
2483891 to
fc88ed1
Compare
|
Race fixed. Ready for review again. |
fc88ed1 to
a653784
Compare
a653784 to
ec6d077
Compare
|
I plan to continue the work on snapshots on shutdown and this PR is a prerequisite for that. I have rebased this PR to remove the conflicts. It is ready for another review now. |
bwplotka
left a comment
There was a problem hiding this comment.
It looks good, just minor nits and one question. Otherwise LGTM!
tsdb/head.go
Outdated
| // could cause race below. So we wait for the goroutine to empty input the buffer. | ||
| idx := mSeries.ref % uint64(n) | ||
| inputs[idx] <- []record.RefSample{} // To ensure that all the old samples are processed. | ||
| for len(inputs[idx]) != 0 { |
There was a problem hiding this comment.
This is first time I see this pattern of checking len on channel, nice. Isn't that racy, when writer is not fast enough to give elements to input[idx] chan and buffer is read faster than writer writes?
There was a problem hiding this comment.
The writer is giving elements to input[idx] in the same loop, and it is serial to this line. So when we are here, there is nothing giving elements to any channels in input slice. So there won't be any read-write race, there is only reading.
|
I will wait for #9147 to be merged before this. |
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
e602326 to
b122c81
Compare
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
| inputs[idx] <- []record.RefSample{} | ||
| for len(inputs[idx]) != 0 { |
There was a problem hiding this comment.
Both of these lines can deadlock; see #9169 for one case.
In this PR I fix a TODO I had put before
// TODO(codesome) Discard old samples and mmapped chunks and use mmap chunks for the new series ID.Assertion:
Basically, if there is a duplicate series record in the WAL, it means that the compaction happened in between and the series including all it's samples were compacted before this duplicate record. Hence, we can discard all that we have replayed till now for that series when we come across a duplicate record.