Skip to content
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

Tombstone memory improvements #7084

Merged
merged 13 commits into from
Jul 29, 2016
Merged

Tombstone memory improvements #7084

merged 13 commits into from
Jul 29, 2016

Conversation

jwilder
Copy link
Contributor

@jwilder jwilder commented Jul 27, 2016

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated

This PR has a number of improvements to reduce memory usage, improve performance and fix bugs related to tombstone files.

  • Tombstones were read into memory and then applied to TSM files. Since we read TSM files concurrently, this could cause large memory spikes at startup. It now loads them iteratively using a more consistent amount of memory.
  • V1 tombstones also read all values into memory. This has been changed to be more iterative.
  • Deletes to TSM files are not applied concurrently which should improve deletes in the engine.
  • When tombstones are loaded, batches are used to apply each tombstone instead of reading them all into memory at once.
  • When tombstones were written, a bug caused the series to be add N times for each TSM files. This cause tombstone files to be much larger than necessary and be much slower to reload.
  • Some TSM cursors were not closed which could cause TSM files used during a query to and also being compacted to leak.

@jwilder jwilder added this to the 1.0.0 milestone Jul 27, 2016
@jsternberg
Copy link
Contributor

jsternberg commented Jul 27, 2016

I only took a look at the query engine portion of this, but that part's LGTM.

}

resC := make(chan res)
var n int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is just a style thing, but since you know you're sending a result for each file, couldn't you get rid of n completely and just create a buffered channel of length len(f.files)?

In your gathering loop at the bottom of walkFiles, you would then loop len(resC) times.

@e-dard
Copy link
Contributor

e-dard commented Jul 28, 2016

Just a few nits, otherwise LGTM 👍

// struct to hold the result of opening each reader in a goroutine
type res struct {
err error
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you could just do a chan error since there's only one field in the struct.

@benbjohnson
Copy link
Contributor

I agree with @e-dard's nits but otherwise LGTM.

}
batch = append(batch, ts.Key)

if len(batch) > 4096 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

len(batch) > 4096 means the original capacity of batch was exceeded and the slice had to grow. Nit, but should this be >= 4096?

jwilder added 13 commits July 28, 2016 20:25
Tombstone were read fully into memory at startup which could consume
a lot of RAM and OOM the process if there were a lot of deleted
series and many TSM files.

This now walks the tombstone file and iteratively applies the tombstone
which uses significantly less RAM.  This may be slightly slower in the
generate cause, but should scale better.
Use a bufio.Scanner to read v1 tombstones instead of reading in
the whole file and parsing it from memory.
This keeps some memory bounds when reloading a TSM files tombstones
so that the heap does not grow exceedintly fast and stay there
after the deletes are applied.
If there were multiple TSM files and a delete/drop was run,
we would write the delete series to the tombstone file N
times for each file.  This occurred because FileStore.WalkKeys walks
every key in every TSM file which can return duplicate keys.

This issue caused TSM files to be much larger than they should be
and also cause large memory usage during the delete.
Aux and condition iterators where not closed which could
cause TSM files to leak if they were queried against while
a compaction was running.
If they were left around, re-enabling them again could cause
future compactions to continuously fail.  A restart of the
server would clean them up correctly though.
break cause the first one to be tracked and all others would
leak as temp files that would not be removed until the server
restarted.
The path info only contained the file name which caused tombstone
files to not be removed if there were queries running against
a file that was compacted.

This is now consistent with the TSMReader.Path which returns the
full path info.
@jwilder jwilder merged commit 37674d2 into master Jul 29, 2016
@jwilder jwilder deleted the jw-tombstones branch July 29, 2016 02:45
@oiooj
Copy link
Contributor

oiooj commented Aug 15, 2016

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants