changefeedccl: use numcpu >> 2 workers for event consumers#89659
changefeedccl: use numcpu >> 2 workers for event consumers#89659craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
| if idealNumber < 1 { | ||
| return 1 | ||
| } | ||
| return int64(idealNumber) |
There was a problem hiding this comment.
Do we have prior art to using defaults that could change if e.g. cluster is upsized/moved?
I think we should change the semantics of this setting: -1 disables, 0 -- we pick reasonable default, anything >0 -- we use user provided value.
When we pick default, we should do this logic; but also, let's cap the max at 8.
There was a problem hiding this comment.
Just pushed updates. I moved the default value calculation to where we initialize the event consumer. This way, we can at least generate a new number whenever a changefeed is started/restarted.
f0284a0 to
bafa186
Compare
9559507 to
29dd10e
Compare
|
Should this instead use |
|
good point, @aaron Zinger ***@***.***>
…On Thu, Oct 13, 2022 at 12:57 PM Aaron Zinger ***@***.***> wrote:
Should this instead use runtime.GOMAXPROCS per the comment here?
https://github.com/jayshrivastava/cockroach/blob/29dd10e49b44ff4023cd06b173b791685c20f268/pkg/util/system/num_cpu.go#L17-L27
—
Reply to this email directly, view it on GitHub
<#89659 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANA4FVD56X6DK3TG7KRCRZ3WDA5N3ANCNFSM6AAAAAARBNS5ZI>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Yes, we should. I'll update it. |
29dd10e to
bedb063
Compare
Previously, a default value of 8 was used for the kvevent parallel consumer. The reason for this was that we observed performance improvements in a 15 node 32 VCPU cluster when we increased this parameter to 8. After 8, the improvements were much smaller. The issue with a default of 8 is that that on smaller machines, 8 workers can be too much overhead, especially since the work is CPU intensive. This change updates the default to be runtime.NumCPU() >> 2 workers, which aligns with using 8 workers on 32 VCPU machines. Fixes cockroachdb#89589 Epic: none Release note: None
bedb063 to
ab840c7
Compare
|
bors r+ |
|
Build succeeded: |
Previously, a default value of 8 was used for the kvevent parallel consumer. The reason for this was that we observed performance improvements in a 15 node 32 VCPU cluster when we increased this parameter to 8. After 8, the improvements were much smaller.
The issue with a default of 8 is that that on smaller machines, 8 workers can be too much overhead, especially since the work is CPU intensive.
This change updates the default to be runtime.NumCPU() >> 2 workers, which aligns with using 8 workers on 32 VCPU machines.
Fixes #89589
Epic: none
Release note: None