-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gossip: delay and batch gossip info propagation #119426
base: master
Are you sure you want to change the base?
gossip: delay and batch gossip info propagation #119426
Conversation
Fixes cockroachdb#119420. This commit addresses cockroachdb#119420 by delaying gossiping updated infos to peers by up to 10ms. In doing so, we more effectively batch info updates and bound the amount of time we spend computing info deltas. With a maxHops = 5, a gossipPropagateInfosDelay of 10ms means that we will be delaying the propagation of info updates by up to 50ms. This should be a reasonable delay for most use cases, given the benefit of this change. TODO: run some tests. Release note (performance improvement): gossip info propagation is now delayed by up to 10ms in order to promote more batching of gossip updates. The effect of this is TBD.
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
FWIW this seems like a pretty clear win, I'd love to see it land @nvanbenschoten . It's nice to see some interest in gossip performance recently. If there's something particular you'd like help with, let me know and I may be able to. I have a bunch of old ideas scattered around for how to optimize parts of this code of varying usefulness, for example splitting up the |
How did you choose 10ms for the delay? Is that an empirically-determined optimium? I can imagine it could be a substantially longer delay – 50ms, or 100ms...? I think balancing it against the rate at which the items being gossiped change, and how sensitive the system is to increasingly levels of staleness in the gosipped info. Also, putting some randomness into the delay probably makes sense. |
@spencerkimball it's not at all empirically determined to be an optimum between staleness and CPU. This came about from some experimentation with a 1,000 node cluster. While at 400 nodes, a 10ms propagation delay was enough to reduce idle cpu load from 0.69 cpus/node (34.8% of each
We've recently bumped up against the upper bounds of tolerable gossip delay on some larger customer clusters (~200 nodes, 1,500 stores). It's on the order of seconds, so I agree that a longer delay (50ms or 100ms) is probably fine. We just need to keep in mind that the per-node delay will be multiplied by the number of hops in the gossip network.
Agreed, a jitter feels appropriate. We're continuing to consider targeted investments in gossip as we look to larger scale clusters, so I imagine we'll take this draft and turn it into something real sometime soon. Footnotes
|
All makes sense. This suggestion from @andrewbaptist feels like it would be extremely consequential at scale: #117393. Would need to implement some custom diff & merge logic to update store descriptors with very partial information, according to the real-time store capacity and usage metrics. |
Fixes #119420.
This commit addresses #119420 by delaying gossiping updated infos to peers by up to 10ms. In doing so, we more effectively batch info updates and bound the amount of time we spend computing info deltas.
With a maxHops = 5, a gossipPropagateInfosDelay of 10ms means that we will be delaying the propagation of info updates by up to 50ms. This should be a reasonable delay for most use cases, given the benefit of this change.
TODO: run some tests.
Release note (performance improvement): gossip info propagation is now delayed by up to 10ms in order to promote more batching of gossip updates. The effect of this is TBD.