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:59:14 UTC
[GitHub] [druid] gianm opened a new pull request #9928: Fix various processing buffer leaks and simplify BlockingPool.
gianm opened a new pull request #9928:
URL: https://github.com/apache/druid/pull/9928
- GroupByQueryEngineV2: Fix leak of intermediate processing buffer when
exceptions are thrown before result sequence is created.
- PooledTopNAlgorithm: Fix leak of intermediate processing buffer when
exceptions are thrown before the PooledTopNParams object is created.
- BlockingPool: Remove unused "take" methods.
----------------------------------------------------------------
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 edited a comment on pull request #9928: Fix various processing buffer leaks and simplify BlockingPool.
Posted by GitBox <gi...@apache.org>.
gianm edited a comment on pull request #9928:
URL: https://github.com/apache/druid/pull/9928#issuecomment-637237895
I added some tests that validate that buffers are always returned to the pool for the standard groupBy and topN suite.
----------------------------------------------------------------
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 #9928: Fix various processing buffer leaks and simplify BlockingPool.
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #9928:
URL: https://github.com/apache/druid/pull/9928#issuecomment-634437937
> > ```
> > * [`aggBuffers` should be closed (or `close()` should be called) when an exception is thrown in `OffheapIncrementalIndex`.](https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/incremental/OffheapIncrementalIndex.java#L55)
> > ```
>
> It looks like this code is only used in groupBy v1 when the undocumented `useOffheap` parameter is used. This is doubly deprecated: v2 has supplanted v1 a while ago, and useOffheap isn't documented. I think we should just delete the offheap implementation.
Oh yeah, sorry I was confused with `OffHeapMemorySegmentWriteOutMedium`. But I remember there was some discussion on using OffheapIncrementalIndex for indexing (or using Oak: #5698). We should probably continue the discussion and draw a conclusion there.
> Similar to #9934 I am down to add tests, I'll just need to come back to this later.
Cool, I'll take another look after you add tests.
----------------------------------------------------------------
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 #9928: Fix various processing buffer leaks and simplify BlockingPool.
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #9928:
URL: https://github.com/apache/druid/pull/9928#issuecomment-634425765
> Do you think it's worth fixing these in this PR too?
>
> * [`bufHolder` will not be closed if `decmopressor.decompress()` throws an exception.](https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/data/DecompressingByteBufferObjectStrategy.java#L49-L56)
IMO this one makes sense to do in a separate PR. This looks like a legitimate issue, but it's a decompression buffer leak, not a processing buffer leak.
> * [`aggBuffers` should be closed (or `close()` should be called) when an exception is thrown in `OffheapIncrementalIndex`.](https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/incremental/OffheapIncrementalIndex.java#L55)
It looks like this code is only used in groupBy v1 when the undocumented `useOffheap` parameter is used. This is doubly deprecated: v2 has supplanted v1 a while ago, and useOffheap isn't documented. I think we should just delete the offheap implementation.
> Similar to #9934, it would be nice to add some unit tests. It seems not hard for `GroupByQueryEngineV2`. Not sure when it's possible to throw an exception before creating `PooledTopNParams` in `PooledTopNAlgorithm`.
Similar to #9934 I am down to add tests, I'll just need to come back to this later.
----------------------------------------------------------------
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 #9928: Fix various processing buffer leaks and simplify BlockingPool.
Posted by GitBox <gi...@apache.org>.
gianm merged pull request #9928:
URL: https://github.com/apache/druid/pull/9928
----------------------------------------------------------------
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 a change in pull request #9928: Fix various processing buffer leaks and simplify BlockingPool.
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9928:
URL: https://github.com/apache/druid/pull/9928#discussion_r430806515
##########
File path: processing/src/main/java/org/apache/druid/query/topn/PooledTopNAlgorithm.java
##########
@@ -243,27 +240,38 @@ public PooledTopNParams makeInitParams(ColumnSelectorPlus selectorPlus, Cursor c
}
};
- final int numBytesToWorkWith = resultsBuf.remaining();
- final int[] aggregatorSizes = new int[query.getAggregatorSpecs().size()];
- int numBytesPerRecord = 0;
+ final ResourceHolder<ByteBuffer> resultsBufHolder = bufferPool.take();
- for (int i = 0; i < query.getAggregatorSpecs().size(); ++i) {
- aggregatorSizes[i] = query.getAggregatorSpecs().get(i).getMaxIntermediateSizeWithNulls();
- numBytesPerRecord += aggregatorSizes[i];
- }
+ try {
+ final ByteBuffer resultsBuf = resultsBufHolder.get();
+ resultsBuf.clear();
+
+ final int numBytesToWorkWith = resultsBuf.remaining();
+ final int[] aggregatorSizes = new int[query.getAggregatorSpecs().size()];
+ int numBytesPerRecord = 0;
- final int numValuesPerPass = numBytesPerRecord > 0 ? numBytesToWorkWith / numBytesPerRecord : cardinality;
-
- return PooledTopNParams.builder()
- .withSelectorPlus(selectorPlus)
- .withCursor(cursor)
- .withResultsBufHolder(resultsBufHolder)
- .withResultsBuf(resultsBuf)
- .withArrayProvider(arrayProvider)
- .withNumBytesPerRecord(numBytesPerRecord)
- .withNumValuesPerPass(numValuesPerPass)
- .withAggregatorSizes(aggregatorSizes)
- .build();
+ for (int i = 0; i < query.getAggregatorSpecs().size(); ++i) {
+ aggregatorSizes[i] = query.getAggregatorSpecs().get(i).getMaxIntermediateSizeWithNulls();
+ numBytesPerRecord += aggregatorSizes[i];
+ }
+
+ final int numValuesPerPass = numBytesPerRecord > 0 ? numBytesToWorkWith / numBytesPerRecord : cardinality;
Review comment:
What line can throw an exception? Or is this more like to be safer?
----------------------------------------------------------------
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 a change in pull request #9928: Fix various processing buffer leaks and simplify BlockingPool.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #9928:
URL: https://github.com/apache/druid/pull/9928#discussion_r430853709
##########
File path: processing/src/main/java/org/apache/druid/query/topn/PooledTopNAlgorithm.java
##########
@@ -243,27 +240,38 @@ public PooledTopNParams makeInitParams(ColumnSelectorPlus selectorPlus, Cursor c
}
};
- final int numBytesToWorkWith = resultsBuf.remaining();
- final int[] aggregatorSizes = new int[query.getAggregatorSpecs().size()];
- int numBytesPerRecord = 0;
+ final ResourceHolder<ByteBuffer> resultsBufHolder = bufferPool.take();
- for (int i = 0; i < query.getAggregatorSpecs().size(); ++i) {
- aggregatorSizes[i] = query.getAggregatorSpecs().get(i).getMaxIntermediateSizeWithNulls();
- numBytesPerRecord += aggregatorSizes[i];
- }
+ try {
+ final ByteBuffer resultsBuf = resultsBufHolder.get();
+ resultsBuf.clear();
+
+ final int numBytesToWorkWith = resultsBuf.remaining();
+ final int[] aggregatorSizes = new int[query.getAggregatorSpecs().size()];
+ int numBytesPerRecord = 0;
- final int numValuesPerPass = numBytesPerRecord > 0 ? numBytesToWorkWith / numBytesPerRecord : cardinality;
-
- return PooledTopNParams.builder()
- .withSelectorPlus(selectorPlus)
- .withCursor(cursor)
- .withResultsBufHolder(resultsBufHolder)
- .withResultsBuf(resultsBuf)
- .withArrayProvider(arrayProvider)
- .withNumBytesPerRecord(numBytesPerRecord)
- .withNumValuesPerPass(numValuesPerPass)
- .withAggregatorSizes(aggregatorSizes)
- .build();
+ for (int i = 0; i < query.getAggregatorSpecs().size(); ++i) {
+ aggregatorSizes[i] = query.getAggregatorSpecs().get(i).getMaxIntermediateSizeWithNulls();
+ numBytesPerRecord += aggregatorSizes[i];
+ }
+
+ final int numValuesPerPass = numBytesPerRecord > 0 ? numBytesToWorkWith / numBytesPerRecord : cardinality;
Review comment:
It's mostly for good practice (when you get a buffer, you better make sure you either free it or pass it along to someone that will free it). I don't think any lines here are very likely to throw exceptions. However, it is possible. I think the most likely is this one:
```java
aggregatorSizes[i] = query.getAggregatorSpecs().get(i).getMaxIntermediateSizeWithNulls();
```
An aggregator could potentially throw an exception from that method for some reason (maybe illegal state, or something). It isn't in the control of the query engine. However, I don't have any examples of where this can actually happen.
----------------------------------------------------------------
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 #9928: Fix various processing buffer leaks and simplify BlockingPool.
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #9928:
URL: https://github.com/apache/druid/pull/9928#issuecomment-637237895
I added some tests that validate that buffers are always returned to the pool for groupBy and topN.
----------------------------------------------------------------
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