Delete series support for TSM #6483
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
|
By analyzing the blame information on this pull request, we identified @joelegasse to be a potential reviewer |
| c.DeleteRange(keys, math.MinInt64, math.MaxInt64) | ||
| } | ||
|
|
||
| // DeleteRange will remove the keys containing points between min and max from the cache |
There was a problem hiding this comment.
This should probably be re-worded to mention that only the values within the given range are removed. Right now, I interpret this to mean the entire key is removed if any value is in that time range.
jwilder
force-pushed
the
jw-delete-series
branch
2 times, most recently
from
914a123 to
1a432f9
Compare
|
Overall it looks really good. There were a couple nits mentioned by @joelegasse but other than that, 👍 . Also, I'm happy that I'm not the only one submitting 2KLOC PRs. :) |
|
@benbjohnson It's mostly test code. :) All comments addressed. I brought back |
This adds support for a time range to tombstone files to allow a subset of points to be deleted instead of the whole series. It changes the tombstone file format to a binary format and maintains backwards compatibility with the old text format tombstone files.
There are two TSMIndex implementations, the directIndex and the indirectIndex. Originally, we only had the directIndex and later added the indirectIndex and NewTSMReaderWithOptions in order to allow both indexes to be used in tests and code. This has created a problem since we really only use the directIndex for writing and always use the indirectIndex for reading. This changes removes the NewTSMReaderWithOptions func so that it is no longer possible to create a TSMReader with a directIndex. This will allow a lot of the block reading code used by the directIndex to be removed and simplify maintainence. It also gives better test coverage of the code that is actually used by the TSM engine now.
No longer used
Allows for future versionion of the TSMIndex as well as removing a lot of unnecessary code.
It's very inneficient and should never be used.
Not used
This will skip blocks that are fully tombstoned as well as remove points that have been removed within a block.
jwilder
force-pushed
the
jw-delete-series
branch
from
46ae9ec to
e7891cb
Compare
|
LGTM 👍 |
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 PR adds the capability to the TSM engine to remove points based on time ranges to support the
DELETE SERIESstatement. The query parser/execution is not part of this PR.The main changes to support this capability are:
indirectIndexto support partially deleted keysThere are a few unrelated changes in the PR done to make the implementation simpler as well as fix some bugs:
directIndexandindirectIndeximplemented the sameTSMIndexinterface which is used for reading and writing. We really only useddirectIndexfor writing and tests andindirectIndexfor reading. These have been split out into separate reader and writer types so that we do not need to continue to implement reading capabilities in thedirectIndexwhen they are not used. This will also make it easier to support different version of indexes in the future when we need to extend the TSM file format.fileAccessor)NewTSMReaderWithOptionswhich allowed creating a TSMReader w/ a direct or indirect index. There is onlyNewTSMReadernow that always usesindirectIndex. Create aTSMReaderwith adirectIndexwas only useful for writing tests that don't write to disk.TSMReaderfuncs that should never be called (e.g.Keys)cc @benbjohnson