Fix TSDB creation conflicting with transfer #1818
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pracucci
force-pushed
the
fix-tsdb-transfer
branch
from
3494f29 to
6f4253e
Compare
thorfour
reviewed
Nov 13, 2019
thorfour
approved these changes
Nov 13, 2019
codesome
reviewed
Nov 13, 2019
pkg/ingester/ingester_v2.go
Outdated
| // a transfer in occurs. | ||
| if !force { | ||
| ingesterState := i.lifecycler.GetState() | ||
| if ingesterState != ring.ACTIVE { |
There was a problem hiding this comment.
Nit: you can combine these 2, also combine it with the above if !force check to be single if statement.
pracucci
force-pushed
the
fix-tsdb-transfer
branch
from
bb44c41 to
d8a317e
Compare
tomwilkie
reviewed
Nov 15, 2019
pkg/ring/model.go
Outdated
| @@ -184,7 +184,7 @@ func (d *Desc) TokensFor(id string) (tokens, other []uint32) { | |||
| func (i *IngesterDesc) IsHealthy(op Operation, heartbeatTimeout time.Duration) bool { | |||
| if op == Write && i.State != ACTIVE { | |||
| return false | |||
| } else if op == Read && i.State == JOINING { | |||
| } else if op == Read && i.State != ACTIVE && i.State != LEAVING { | |||
There was a problem hiding this comment.
IIRC pending ingesters shouldn't have any tokens; how did you trigger this?
There was a problem hiding this comment.
It should occur in the paths where we use forAllIngesters(). I think one path is:
- Querier receive a
LabelNamesrequest - Querier forwards the request to the distributor
- The distributor runs
client.LabelNames()indistributor.forAllIngesters() distributor.forAllIngesters()callsring.GetAll()ring.GetAll()returns all ingesters which satisfy theIsHealthy()
There was a problem hiding this comment.
@bboreham What's your take on this? If this change is a problem, I could rollback it in order to move one with this PR.
There was a problem hiding this comment.
Thanks for enumerating these.
codesome
approved these changes
Nov 15, 2019
pkg/ingester/ingester_v2_test.go
Outdated
Comment on lines
120
to
121
| testData := testData | ||
|
|
pracucci
force-pushed
the
fix-tsdb-transfer
branch
2 times, most recently
from
748dcbb to
7a81a93
Compare
|
Nice work! I appreciate the refactoring on the read path ( |
pracucci
force-pushed
the
fix-tsdb-transfer
branch
from
7a81a93 to
6e4635a
Compare
3 tasks
gouthamve
approved these changes
Dec 2, 2019
|
You got some merge conflicts now |
pracucci
force-pushed
the
fix-tsdb-transfer
branch
from
1495357 to
fadbbdc
Compare
|
@gouthamve I've fixed the conflicts after rebasing. May you help me moving forward with this PR, please? |
gouthamve
approved these changes
Dec 5, 2019
|
Active development going on here :) you got more conflicts |
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
…n JOINING state Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
…ll require a dedicated discussion Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
pracucci
force-pushed
the
fix-tsdb-transfer
branch
from
fadbbdc to
772ed25
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does:
In this PR:
Suggest to enable "Hide whitespace changes" when reviewing for an easier diff.
Why?
I found an issue leading to data loss and/or TSDB wal corruption, which should be fixed by this PR. What happens:
PENDINGstate, so a query request can hit an ingester even if theACTIVEstate has not been reached yet (not desired, fixed)If a query hit a new ingester in the
PENDINGstate before the transfer starts (it switches toJOININGthen), the query creates a TSDB opening the wal segment00000000. The subsequent transfer will overwrite the wal segments, while the00000000file reference is still kept by the opened TSDB.Moreover, if it get transferred a wal where a checkpoint already occurred, the
00000000segment is not part of the transfer but the segment 0 is created by the opened TSDB which leads to an inconsistent segments numbering on disk (ie. 0, checkpoint.6, 7, 8, 9) which cause TSDB to fail a subsequent re-opening.Which issue(s) this PR fixes:
No reported issue.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]