You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/05/27 03:54:03 UTC
[GitHub] [druid] gianm opened a new pull request #9934: Fix various Yielder leaks.
gianm opened a new pull request #9934:
URL: https://github.com/apache/druid/pull/9934
- CombiningSequence leaked the input yielder from "toYielder" if it ran
into an exception while accumulating the last value from the input
yielder.
- MergeSequence leaked input yielders from "toYielder" if it ran into
an exception while building the initial priority queue.
- ScanQueryRunnerFactory leaked the input yielder in its
"priorityQueueSortAndLimit" strategy if it ran into an exception
while scanning and sorting.
- YieldingSequenceBase.accumulate chomped IOExceptions thrown in
"accumulate" during yielder closing.
The first bug (in CombiningSequence) led to periodic stalls in CalciteQueryTest, because it led to a merge buffer leak in testMaxSubqueryRows.
I'm not sure if any of these bugs have production impacts, but, they might.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #9934: Fix various Yielder leaks.
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #9934:
URL: https://github.com/apache/druid/pull/9934#issuecomment-634418378
Oh CI actually doesn't like the low test coverage 😅
```
ERROR: Insufficient line coverage of 35% (19/53). Required 65%.
ERROR: Insufficient branch coverage of 50% (2/4). Required 65%.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #9934: Fix various Yielder leaks.
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #9934:
URL: https://github.com/apache/druid/pull/9934#issuecomment-637221128
Added various tests, and fixed another bug that occurs when `yielder.get()` throws an exception while MergeSequence is creating the initial priority queue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #9934: Fix various Yielder leaks.
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #9934:
URL: https://github.com/apache/druid/pull/9934#issuecomment-634359443
> Nice catch. Do you think we can add some unit tests for these fixes? It seems not hard at least for `CombiningSequence`, `MergeSequence`, and `YieldingSequenceBase`.
I agree it'd be good, I just didn't have time this afternoon 🙂
I can come back to this a bit later and add tests, though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm merged pull request #9934: Fix various Yielder leaks.
Posted by GitBox <gi...@apache.org>.
gianm merged pull request #9934:
URL: https://github.com/apache/druid/pull/9934
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org