ESQL: Account for field readers in breaker #140666
Conversation
This seaks to prevent out of memory errors by accounting for the memory usage of field readers. We don't have measurements for the actual memory usage, so instead we add an estiamte to the breaker. An overestimate, hopefully. This makes us circuit break when loading many many many fields on small heaps rather than crashing with an out of memory. This also allows readers to cache things a little more aggressively, so long as they are willing to circuit break when receiving a huge number of requests.
| } | ||
|
|
||
| interface Reader { | ||
| interface Reader extends Releasable { |
There was a problem hiding this comment.
Here's the main change in the PR. Everything else is in service of making this Releasable and passing in CircuitBreaker.
| * This implies that we need to load the value from _source. This however is very slow, especially when synthetic source is enabled. | ||
| * We're better off reading from doc_values and converting to BytesRef to satisfy the checker. This is what this block loader is for. | ||
| */ | ||
| static final class BytesRefFromLongsBlockLoader extends BlockDocValuesReader.DocValuesBlockLoader { |
| ); | ||
| } | ||
|
|
||
| public static class DateRangeDocValuesLoader extends BlockDocValuesReader.DocValuesBlockLoader { |
| } | ||
|
|
||
| @Override | ||
| public StoredFieldsSpec rowStrideStoredFieldSpec() { |
There was a problem hiding this comment.
Moved these methods so the inner classes are at the end of the file.
| * Circuit breaker space reserved for each reader. Measured in heap dumps | ||
| * around from 3.5kb to 65kb. This is an intentional overestimate. | ||
| */ | ||
| public static final long ESTIMATED_SIZE = ByteSizeValue.ofKb(100).getBytes(); |
There was a problem hiding this comment.
This 100kb is pretty controversial I think. Will want some discussion. I'd far prefer to have a good estimate, but I don't see a way to do so right now. I've seen the readers 1kb keyword fields be this large.
There was a problem hiding this comment.
If SortedSetDocValues and friends would implement Accountable then we would have much better insight in the on heap memory usage. However I think this is also difficult as the memory usage might be variable as SortedSetDocValues gets used.
Additionally, should we make this configurable via a query pragma or something else?
There was a problem hiding this comment.
Accountable
Yeah. I'd be ok doingif (thingy instanceof Accountable)here. I'd prefer not, but sometimes we can't have all we want.
Additionally, should we make this configurable via a query pragma or something else?
Hmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmm.
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Hi @nik9000, I've created a changelog YAML for you. |
| @Override | ||
| public AllReader reader(LeafReaderContext context) throws IOException { | ||
| public AllReader reader(CircuitBreaker breaker, LeafReaderContext context) throws IOException { | ||
| breaker.addEstimateBytesAndMaybeBreak(ESTIMATED_SIZE, "load blocks"); |
There was a problem hiding this comment.
getSortedNumericDocValues can throw an IOException, so we might leak the memory requested here. I think we need a try/finally block. Also, should we acquire memory in the Reader's constructor and release it in close()? For example, in BooleansBlockDocValuesReader, we release memory - should we also acquire it in the constructor?
There was a problem hiding this comment.
Good point Nhat. I think applies for all getXXXDocValues(...) invocation and so try/finally blocks are also needed else where.
There was a problem hiding this comment.
Damn. I should write a test that hits that.
I had thought "this can throw if the index is tragically corrupted" so, like, if it threw we were toast anyway. But I'll fix it.
martijnvg
left a comment
martijnvg
left a comment
There was a problem hiding this comment.
Thanks Nik, I think Nhat's comment should be addressed, but otherwise LGTM.
| @Override | ||
| public AllReader reader(LeafReaderContext context) throws IOException { | ||
| public AllReader reader(CircuitBreaker breaker, LeafReaderContext context) throws IOException { | ||
| breaker.addEstimateBytesAndMaybeBreak(ESTIMATED_SIZE, "load blocks"); |
There was a problem hiding this comment.
Good point Nhat. I think applies for all getXXXDocValues(...) invocation and so try/finally blocks are also needed else where.
| * Circuit breaker space reserved for each reader. Measured in heap dumps | ||
| * around from 3.5kb to 65kb. This is an intentional overestimate. | ||
| */ | ||
| public static final long ESTIMATED_SIZE = ByteSizeValue.ofKb(100).getBytes(); |
There was a problem hiding this comment.
If SortedSetDocValues and friends would implement Accountable then we would have much better insight in the on heap memory usage. However I think this is also difficult as the memory usage might be variable as SortedSetDocValues gets used.
Additionally, should we make this configurable via a query pragma or something else?
| * shrug because we don't know what the script will do, and we don't know how many doc | ||
| * values it'll load. And, we're not sure much memory the script itself will actually use. | ||
| */ | ||
| public static final long ESTIMATED_SIZE = ByteSizeValue.ofKb(300).getBytes(); |
There was a problem hiding this comment.
Scripts have additional heap memory overhead... We can always adjust it down.
4 participants
This seeks to prevent out of memory errors by accounting for the memory usage of field readers. We don't have measurements for the actual memory usage, so instead we add an estimate to the breaker. An overestimate, hopefully.
This makes us circuit break when loading many many many fields on small heaps rather than crashing with an out of memory.
This also allows readers to cache things a little more aggressively, so long as they are willing to circuit break when receiving a huge number of requests.