Fix TDigestState.read CB leaks#114303
Conversation
ivancea
added
>test
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Just fixed the issue. Or an issue at least, but it makes sense that it was this because it's on read(), and it was only happening in cluster tests apparently. And in a very specific case (CB failing exactly in the |
ivancea
changed the title
ivancea
added
>bug
and removed
>test
|
Hi @ivancea, I've created a changelog YAML for you. |
| } | ||
| for (int i = 0; i < n; i++) { | ||
| state.add(in.readDouble(), in.readVLong()); | ||
| try { |
There was a problem hiding this comment.
I would prefer a try / finally block and use a boolean variable to control the outcome of the execution as we do in other places. See
There was a problem hiding this comment.
Updated. Also, changed the internal catch to a finally too. merging both try-finally in one made the code a bit more complex, so left them separated
There was a problem hiding this comment.
Thanks you, that's looks good.
| try (TDigestState example = TDigestState.createOfType(newLimitedBreaker(ByteSizeValue.ofMb(100)), digestType, 100)) { | ||
| return TDigestState.createUsingParamsFrom(example); | ||
| } | ||
| }); |
There was a problem hiding this comment.
If you use the cranky breaker service it'll blow up randomly and you can assert that you always freed the memory, no matter the random explosion. I rather like it for this sort of thing.
Which one of your tests hits a the breaker exception you are guarding?
There was a problem hiding this comment.
Which one of your tests hits a the breaker exception you are guarding?
The testReadWithData(). The reserve is only called when it has data in it. So I added that one explicitly.
About the cranky breaker, I'm not sure we want it to break randomly. This has multiple potential breaking points I wanted to test (Add memory and release on error, on every constructor).
The cranky breaks 1/20 times. If we're fine with this one (The full suite takes around 1 second on my machine), I would leave it as-is. It tests every potential breaking point (Divided in 16 random seeds).
Is it too much? Not sure
There was a problem hiding this comment.
About the cranky breaker, I'm not sure we want it to break randomly. This has multiple potential breaking points I wanted to test (Add memory and release on error, on every constructor).
That's where the cranky is really good at.
There was a problem hiding this comment.
Just updated it to use the cranky breaker. It will fail ~1/5 times, so I guess it's ok. I suppose even 1/20 times would be ok for something like this.
I guess I was reinventing the wheel here XD
Closes elastic#114194 Fixes `TDigestState.read()` CB leak on error on `.reserve()`.
5 participants
Closes #114194
Fixes
TDigestState.read()CB leak on error on.reserve().