Uh oh!
There was an error while loading. Please reload this page.
[BEAM-9822] Disable grouping when streaming - #11532
Conversation
8b56f44 to
865a11bCompare865a11b to
573bfb9CompareThere was a problem hiding this comment.
I was wondering if this condition needs to be based on the input passed to this stage or based on some parameter from the user?
There was a problem hiding this comment.
it's kinda both!
If the source is unbounded (streaming) - and the groupingFactor has not been specified by the user, then default to no grouping.
There was a problem hiding this comment.
Is there any chance that someone using SpannerIO in a streaming pipeline is relying on the default grouping factor being 1000? I'm concerned this backwards-incompatible change could break them. Would it be sufficient to just give users the option to disable batching by setting the grouping factor to 1?
There was a problem hiding this comment.
They already can set groupingFactorb to 1 if they want...
Breaking backward compatibility: unlikely.
The default of 1000 causes OOMs when using streaming, with wide windows, and high throughput... When this happens, it is not always obvious that grouping is the issue...
With smaller windows/less throughput, it is much less likely that a group will be filled, (groups are bounded by bundles, which are bounded by windows)., So it is unlikely that anyone ever got to fill the group with 1000 batches.
There was a problem hiding this comment.
They already can set groupingFactorb to 1 if they want...
Ha yeah sorry that was unclear. At the time I thought that groupingFactor = 1 enabled the optimization in #11529, so I was wondering if this was really necessary since users could just enable them by setting grouping factor manually. But I see now that grouping is separate from batching. And its disabling batching that enables your other PR.
573bfb9 to
cecb959Compareallenpradeep
commented
May 6, 2020
LGTM. |
9e56c08 to
3cc214bComparenielm
commented
May 18, 2020
Retest this please |
1 similar comment
TheNeuralBit
commented
May 18, 2020
Retest this please |
TheNeuralBit
commented
May 18, 2020
Looks like you need to run spotless to auto-format. You can use |
Grouping adds significant latency and memory use, and when streaming this causes both OOMs and high pipeline latencies.
3cc214b to
d64df6aComparenielm
commented
May 19, 2020
Retest this please |
nielm
commented
May 19, 2020
|
TheNeuralBit
commented
May 19, 2020
Retest this please |
1 similar comment
TheNeuralBit
commented
May 19, 2020
Retest this please |
| .orElse( | ||
| input.isBounded() == IsBounded.BOUNDED | ||
| ? DEFAULT_GROUPING_FACTOR | ||
| : 1), |
There was a problem hiding this comment.
It would be nice if this were another constant. We could have DEFAULT_GROUPING_FACTOR_BOUNDED and DEFAULT_GROUPING_FACTOR_UNBOUNDED. It doesn't need to be done here, could be in a follow-up PR.
TheNeuralBit
left a comment
There was a problem hiding this comment.
LGTM, thanks! I'll go ahead and merge this, but can you add a note about this in CHANGES.md in another PR?
Grouping adds significant latency and memory use.
When streaming, end-to-end pipeline latency is important, and many worker threads are executed, meaning that OOM's can frequently occur.
This PR disables grouping by default in streaming mode, ensuring lower memory use and faster end-end latency.
Note, this PR is dependent on PR #11528
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.