[I] Broker can allocate excessive processing buffers (druid)

["gianm (via GitHub)" <gi...@apache.org>]

gianm opened a new issue, #20494:
URL: https://github.com/apache/druid/issues/20494

   The Broker runs a query itself, through LocalQuerySegmentWalker, when all of its datasources are global (e.g. inline, lookup, subquery, etc). On this path the Broker uses the same per-segment engines as data servers, but running on the query thread (the HTTP request handling thread) rather than a bounded processing pool. This can lead to excessive allocation of processing buffers, to the degree that the query engines request them.
   
   As a workaround, people can lower `druid.processing.buffer.sizeBytes` or raise `-XX:MaxDirectMemorySize` on Brokers. If doing the first, it may be useful to also enable disk spilling with `druid.query.groupBy.maxOnDiskStorage`.
   
   Possible fixes:
   
   1. Give the Broker a bounded-size local processing pool, and run processing tasks there rather than in HTTP threads.
   2. Allocate processing buffers by slicing up merge buffers, rather than allocating from an on-demand processing buffer pool. (Queries would wait for buffers to become available.)


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org.apache.org

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

Re: [I] Broker can allocate excessive processing buffers (druid)

["Nagarjunanm (via GitHub)" <gi...@apache.org>]

Nagarjunanm commented on issue #20494:
URL: https://github.com/apache/druid/issues/20494#issuecomment-6264882489

   Hi @gianm, I'd like to pick this up, if nobody else is working on it.
   
   I reproduced it with a test that uses the Broker's real buffer pools. With 1 MB buffers, 4 merge buffers, and direct memory set to exactly what the startup check requires (5 MB), 6 concurrent groupby's on an inline datasource allocated 4 processing buffers, so 8 MB were in use.
   
   I'm planning to go with option 1:
   
   - Add a Broker setting, `druid.processing.local.numThreads`, defaulting to 1. Local queries would run their per-segment work on a `PrioritizedExecutorService` with that many threads instead of in HTTP threads, so at most that many processing buffers are in use at once.
   - Change the Broker's direct memory check and the docs to `sizeBytes * (numMergeBuffers + numThreads)`. With the default of 1, that's the current formula, so no Broker fails its startup check after upgrading.
   - Keep `query/cpu/time` accurate by adding the CPU time of the pool threads.
   
   With 1 thread, concurrent local groupBy, timeseries, and topN queries would be processed one at a time. Is that default OK with you, or would you rather size the pool another way? And does option 1 work for you, or would you rather go with option 2?


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

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

Re: [I] Broker can allocate excessive processing buffers (druid)

["FrankChen021 (via GitHub)" <gi...@apache.org>]

FrankChen021 commented on issue #20494:
URL: https://github.com/apache/druid/issues/20494#issuecomment-6266448562

   I think option 2 should be better, I don't want another configuration. Current configurations for merge buffers/processing threads are already complicated and these static allocated memory is one thing which I think is not flexible enough.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

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

Re: [I] Broker can allocate excessive processing buffers (druid)

["Nagarjunanm (via GitHub)" <gi...@apache.org>]

Nagarjunanm commented on issue #20494:
URL: https://github.com/apache/druid/issues/20494#issuecomment-6268062562

   Thanks for the feedback and yes having lesser configuration make sense which is in inline with how Druid has evolved over years even in ingestion front as well.
   
   Here is the rough thought on the same [ option 2 ]:
   
   - When the Broker runs a query itself, it would take the memory from its merge buffers instead of keeping a separate processing buffer pool. Its memory check would then only count merge buffers, so no Broker would need more memory than it does today.
   - Groupby is the main query type that needs a processing buffer here. The catch is that it's already holding a merge buffer while it runs. If it then had to wait for a second merge buffer, queries could end up stuck waiting on each other. So instead, it would split the merge buffer it already holds, using part of it for processing and the rest for merging.
   - Timeseries only needs a buffer when it vectorises, so it would borrow a merge buffer in that case and wait if none is free. That's safe because it isn't holding any other buffer while it waits. topN doesn't need one I believe.
   
   Follow up: Is it fine with groupBy splitting the merge buffer it already holds? The other option is for each local groupby to take an extra merge buffer at the same time as its first one, when the query starts. That avoids the waiting problem and keeps buffers full-size, but each local groupBy would need one more merge buffer than it does now.
   
   Please do let me know if I'm missing anything here. 


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

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