You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "gianm (via GitHub)" <gi...@apache.org> on 2023/07/11 00:16:08 UTC
[PR] AggregatorFactory: Use guessAggregatorHeapFootprint when factorizeWithSize is not implemented. (druid)
gianm opened a new pull request, #14567:
URL: https://github.com/apache/druid/pull/14567
There are two ways of estimating heap footprint of an Aggregator:
1) AggregatorFactory#guessAggregatorHeapFootprint
2) AggregatorFactory#factorizeWithSize + Aggregator#aggregateWithSize
When the second path is used, the default implementation of factorizeWithSize is now updated to delegate to guessAggregatorHeapFootprint, making these equivalent. The old logic used getMaxIntermediateSize, which is less accurate.
Also fixes a bug where, when using the second path, calling factorizeWithSize on PassthroughAggregatorFactory would fail because getMaxIntermediateSize was not implemented. (There is no buffer aggregator, so there would be no need.)
--
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: [PR] AggregatorFactory: Use guessAggregatorHeapFootprint when factorizeWithSize is not implemented. (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on PR #14567:
URL: https://github.com/apache/druid/pull/14567#issuecomment-1632642729
@LakshSingla I think you're right that the exception should ideally be a `DEFENSIVE`. We expect this never to happen, which is classic defensive. The fact that it was seen at one point is a bug that this PR fixes. My only note really would be that, IMO, the "Treat this exception as a bug…" is unnecessary. I'm not sure if we want the message, but if we do, it would actually make sense on _every_ `DEFENSIVE` exception so would make more sense to bake into the `DEFENSIVE` exception generator or mapper itself.
--
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: [PR] AggregatorFactory: Use guessAggregatorHeapFootprint when factorizeWithSize is not implemented. (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm merged PR #14567:
URL: https://github.com/apache/druid/pull/14567
--
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