Fix index inconsistency after deletes #8576
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
e-dard
suggested changes
Jul 7, 2017
tsdb/engine/tsm1/engine.go
Outdated
| } | ||
|
|
||
| // onFileStoreReplace is callback handler invoked when the FileStore | ||
| // has replace one set of TSM files with a new set. |
tsdb/engine/tsm1/engine.go
Outdated
| readers = append(readers, ch) | ||
|
|
||
| go func(c chan seriesKey, r TSMFile) { | ||
| n := r.KeyCount() |
There was a problem hiding this comment.
nit: you could just put i < r.KeyCount()
There was a problem hiding this comment.
KeyCount ends up taking RLocks on the reader and I didn't want to lock the readers with every loop iteration.
tsdb/engine/tsm1/engine.go
Outdated
| e.Cache.ApplyEntryFn(func(key string, entry *entry) error { | ||
| fieldType, err := entry.values.InfluxQLType() | ||
| if err != nil { | ||
| // ignore |
There was a problem hiding this comment.
Could we at least log any errors here, or are we expecting them?
There was a problem hiding this comment.
There shouldn't be any errors here. I can log them though.
tsdb/engine/tsm1/engine.go
Outdated
| return nil | ||
| }) | ||
|
|
||
| return |
| @@ -895,7 +895,7 @@ func (d *indirectIndex) OverlapsTimeRange(min, max int64) bool { | |||
|
|
|||
| // OverlapsKeyRange returns true if the min and max keys of the file overlap the arguments min and max. | |||
| func (d *indirectIndex) OverlapsKeyRange(min, max string) bool { | |||
| return d.minKey <= max && d.maxKey >= max | |||
| return d.minKey <= max && d.maxKey >= min | |||
There was a problem hiding this comment.
This looks like it could have been the source of other bugs unrelated to the dropping/missing data issue. Do we have any other open issues that could be hitting this, and how do you think they would manifest themselves?
There was a problem hiding this comment.
This is only called via DeleteRange and would appear that a delete didn't actually delete everything.
The in-memory index can get out of sync when deletes and writes to the same measurement are running concurrently. The index is updated independently from data on disk and it's possible for the index to unassign a shard when data still exists on disk. What happens is that there are TSM files on disk, but the index does not know that the series that exist in those files still are in the shard. Restarting the server reloads the index and the data is visible again. From and end user perspective, this can look like more data is deleted than should have been or that deleted data re-appears after a restart or writes to the shard occur again. There isn't an easy way to resolve this since the index and storage are not transactional resources and we cannot atomically commit or rollback changes to both at once. As a workaround, after new TSM files are installed, we refresh the index with series keys that exist in the new tsm files as well as any lingering data still in the cache. There is a small window of time when the index may be missing series, but it will re-appear after the refresh completes.
The min key was not used in OverlapsKeyRange which caused it to return false when it should be true. This causes a bug where deletes would not write tombstones for files that actually contained the data it was supposed to delete.
This was referenced Jul 7, 2017
|
@e-dard All fixed. |
e-dard
approved these changes
Jul 7, 2017
This was referenced Jul 7, 2017
Merged
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.
Required for all non-trivial PRs
This fixes an issue where the inmem index would be inconsistent with the data stored on disk. It also fixes a bug where deletes do not write tombstones for data that should be deleted.
The inconsistency can occur under write load to a measurement and concurrently dropping the measurement. There is a race where the incoming writes create index entries, but the writes have not hit the cache/wal yet. The delete completes and removes them series from the index and then the write completes and stores the data on disk. The end result is that the data is on disk, but the index is inconsistent and is not aware that it exists within the shard. Restarting the server causes the index to be reloaded and the data appears again.
There wasn't a good way to fix this without significantly reworking how the index and engine work together or introducing a lot of lock contention mainly to handle this cases of deletes. I went with an approach that refreshes the index from the newly compacted files as well as what is remaining in the cache when the files are installed for use. This resolves the issue, but there can be a brief period of time between when the the delete removes the series from the index and before the index is refreshed where series can appear missing.