-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Compaction fixes #6653
Compaction fixes #6653
Conversation
By analyzing the blame information on this pull request, we identified @joelegasse to be a potential reviewer |
@aderumier @daviesalex @easyrasta if you have a dataset that can easily reproduce #6652, would you be able to test this PR on it? |
e80dcf5
to
0b25541
Compare
So this fixed the panic, but not the missing data. The startup was very fast, but much of our data (most really) only exists from midnight on the 16th (not all , though). See example data: FWIW, during the startup, most of the time (by a long way) is spent in sync/atomic.AddUint32. This compares to runtime.mapiternext and runtime.indexbytebody in 0.13 stable. I submitted #6658 to track this:
|
Log showing the start with this PR (branch unknown in log, but it was built from branch jw-compact-fix - 0b2554) that works but is missing most data, and then a start with the 0.13 stable release at https://gist.github.com/daviesalex/f9e70754fb2c0ac85454b6db341b34a4 which works and has the data. |
return err | ||
} | ||
|
||
return ErrMaxBlocksExceeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems redundant since there is an err != nil
check below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind the redundant return but I think specifying ErrMaxBlocksExceeded
twice is redundant. You can just do a return err
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, stylistically I prefer grouping the entire thing together so the err
is confined to a specific block:
if err := w.WriteBlock(key, minTime, maxTime, block); err == ErrMaxBlocksExceeded {
if err := w.WriteIndex(); err != nil {
return err
}
return ErrMaxBlocksExceeded
} else if err != nil {
return err
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/return ErrMaxBlocksExceeded/return err
though 😉
Just a nit, but LGTM 👍 |
@jwilder It's starting fine with this PR. (still around 10min of startup time) |
// Filter out only the values for overlapping block | ||
v = Values(v).Include(first.minTime, first.maxTime) | ||
if len(v) > 0 { | ||
// Recoder that we read a subset of the block |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this meant to say, "Record"?
Minor nits but lgtm 👍 |
@daviesalex With the same dataset/host, does 0.13 load all the data? I'm not sure why you would be missing data using this branch as it does not have any changes related to startup. |
Also, were there any errors in the logs? |
@jwilder yes. 0.13 loads all the data. This branch does not (nor does the nightly). The full log (a start under this version, which fails to load all the data, and 0.13, which does) is in the gist above - https://gist.github.com/daviesalex/f9e70754fb2c0ac85454b6db341b34a4 |
@daviesalex If the current nightly has missing data, then it's not caused by this PR. Any chance you could Would you be able to test that commit and just before it? |
I was able to reproduce the missing data on startup. It is due to 0dbd489#diff-82e4e65bd72f00208a49c4797d466ecdR383. |
3fa355e
to
b5cfb0e
Compare
@daviesalex I pushed a fix for the data not reloading on this branch. Let me know if that resolves it for you. |
@jwilder good news. Confirmed that data is there.
Startup down from about 55 minutes to about 15. I tested on two machines and saw the same just over 3x speedup. Great success! Thank you. |
If a large series contains a point that is overwritten, the compactor would load the whole series into RAM during a full compaction. If the series was large, it could cause very large RAM spikes and OOMs. The change reworks the compactor to merge blocks more incrementally similar to the fix done in #6556. Fixes #6557
Switched the max keys test to write int64 of the same value so RLE would kick in and the file size will be smaller (84MB vs 3.8MB). Removed the chunking test which was skipped because the code will not downsize a block into smaller chunks now. Skip MaxKeys tests in various environments because it needs to write too much data to run reliably.
* tsm1 engine test is no longer needed as it's the default now * go1.6 build from git is not needed as we build with go1.6.2 released version now.
The optimization to speed up shard loading had the side effect of skipping adding series to the index that already exist. The skipping was in the wrong location and also skipped the shards measurementFields index which is required in order to query that series in the shard.
Required for all non-trivial PRs
This PR re-introduces the fix for #6556 that was reverted in #6637. The original fix could drop points during compactions as well as cause an
interface conversion
panic (#6652) in some cases. What happened was that merged values from one series could get mixed with the values for the next series. If the series were different types, a panic could occur. If it was the last block to be merged, the values would be lost. Both of these issues should be fixed now as well as the original issue of overwriting a point in a large series triggering memory spikes.This also fixes #6406 that can occur if enough points are written into a single series to cause the TSM index to exceed the max index entries allotment. It will now handle that error and rollover to a new file.