[ES|QL] Convert PackedValuesBlockHash.bytes to BreakingBytesRefBuilder for better memory tracking#140171
Conversation
|
Hi @fang-xing-esql, I've created a changelog YAML for you. |
fang-xing-esql
changed the title
PackedValuesBlockHash.bytes to BreakingBytesRefBuilder for better memory trackingPackedValuesBlockHash.bytes to BreakingBytesRefBuilder for better memory tracking
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| // close bytesRefHash and bytes to prevent memory leaks in case of the initialization fails | ||
| close(); | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
This is valid and fine, but we usually write this as:
boolean success = false;
try {
this.bytesRefHash = HashImplFactory.newBytesRefHash(blockFactory);
this.bytes = new BreakingBytesRefBuilder(blockFactory.breaker(), "PackedValuesBlockHash", this.nullTrackingBytes);
} finally {
if (success == false) {
close();
}
Mostly out of paranoia around eating the stack trace for e. This is just the more "normal" way of writing this for us.
| * , which is reused for each grouping set will trigger CBE. CBE happens when adding around 25th group to bytes. | ||
| */ | ||
| public void testCircuitBreakerWithManyGroups() { | ||
| CircuitBreaker breaker = new MockBigArrays.LimitedBreaker(CircuitBreaker.REQUEST, ByteSizeValue.ofBytes(220000)); |
There was a problem hiding this comment.
I think this test will fail even without the BreakingBytesRefBuilder. You could force it by passing a second CircuitBreaker into the ctor for PackedValuesBlockHash - the second one just for the bytes scratch. Then in prod code you'd just send the circuit breaker in twice. It's.... annoying. But it's nice and paranoid.
| } | ||
|
|
||
| // For circuit breaker testing only {@code PackedValuesBlockHashCircuitBreakerTests} | ||
| PackedValuesBlockHash(List<GroupSpec> specs, BlockFactory blockFactory, CircuitBreaker circuitBreaker, int emitBatchSize) { |
There was a problem hiding this comment.
The primary ctor can call this one like this(specs, blockFactory, blockFactory.circiutBreaker(), emitBatchSize).
There was a problem hiding this comment.
Good point! More refactor is needed. :)
|
Thanks for the review @nik9000 ! |
…lder` for better memory tracking (elastic#140171) * converted PackedValuesBlockHash.bytes to BreakingBytesRefBuilder
fang-xing-esql
deleted the
PackedValuesBlockHash-BreakingBytesRefBuilder
branch
3 participants
It is found during investigating OOM triggered by new heap attack tests added by #139058. Converting
PackedValuesBlockHash.bytestoBreakingBytesRefBuilderdoesn't fix the OOM directly, however it does improve the memory tracking for queries that have long grouping keys.