Use same max-output-buffer-size config for all types of output buffers #8426
Conversation
✅ Deploy Preview for meta-velox canceled.
|
facebook-github-bot
added
the
CLA Signed
|
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
mbasmanova
force-pushed
the
output-buffer-max-size
branch
from
5c141c8 to
224d581
Compare
|
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
mbasmanova
force-pushed
the
output-buffer-max-size
branch
from
224d581 to
9f58663
Compare
|
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| uint64_t maxBufferSize( | ||
| const core::QueryConfig& config, | ||
| PartitionedOutputNode::Kind bufferKind) { | ||
| if (bufferKind == PartitionedOutputNode::Kind::kArbitrary) { | ||
| return config.maxArbitraryBufferSize(); | ||
| } | ||
|
|
||
| return config.maxPartitionedOutputBufferSize(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Need to doublecheck why we had these two separated.
Can this cause any unexpected problem?
|
@mbasmanova merged this pull request in ec3bb6b. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
We need 2 configuration settings to control the behavior of PartitionedOutput
operator.
First, a limit on how many bytes to buffer inside the PartitionedOutput operator
itself. That limit is split N ways (where N is number of partitions) to compute
per-destination packet size. A packet here is a SerializedPage.
Second, a limit on how many bytes to buffer in OutputBuffer across all
destinations (a sum of all ready packets / SerializedPages). This limit needs
to be significantly higher than the first limit.
We have introduced the second limit earlier to use for arbitrary buffer, but it
should be used for all kinds of buffers (partitioned, broadcast and
arbitrary).
This change is to start using this second limit for all kinds of buffers. A
series of follow-up changes needs to rename the configuration property to
remove 'arbitrary' from the name in a backwards compatible way
(max_arbitrary_buffer_size -> max_buffer_size).