Page MenuHomePhabricator

Add a "metronome" for spreading service call load
ClosedPublic

Authored by epriestley on Feb 5 2019, 1:26 AM.

Details

Summary

Ref T13244. See D20080. Rather than randomly jittering service calls, we can give each host a "metronome" that ticks every 60 seconds to get load to spread out after one cycle.

For example, web001 ticks (and makes a service call) when the second hand points at 0:17, web002 at 0:43, web003 at 0:04, etc.

For now I'm just planning to seed the metronomes randomly based on hostname, but we could conceivably give each host an assigned offset some day if we want perfectly smooth service call rates.

Test Plan

Ran unit tests.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Feb 5 2019, 1:26 AM
epriestley requested review of this revision.Feb 5 2019, 1:28 AM
epriestley added a child revision: Restricted Differential Revision.Feb 5 2019, 3:17 AM
amckinley accepted this revision.Feb 5 2019, 7:57 PM
amckinley added inline comments.
src/infrastructure/util/PhabricatorMetronome.php
53

Is this just to enforce setting a frequency before setting an offset? If so, why doesn't setOffset() have the same code?

54

Wow this is a cool function!

howneatisthat

67

I agree that this implementation makes the most sense, but because this code is intended to produce a uniform distribution and because I spent a lot of money to get a CS degree, I feel obligated to point out that certain values for frequency (in particular, powers of 2) can end up generating a very non-uniform distribution of offsets that is significantly worse than the birthday paradox would imply. There's also nothing we can really do in the general sense to fix this other than something like rounding $frequency up to the closest prime number or something equally silly.

If we wanted to totally change the interface for this, we could require that you initialize a metronome with an array of all the objects that will be using it, and assign offsets internally to enforce the most uniform-possible distribution, but that would make this way more unwieldy.

This revision is now accepted and ready to land.Feb 5 2019, 7:57 PM
epriestley added inline comments.Feb 5 2019, 8:08 PM
src/infrastructure/util/PhabricatorMetronome.php
53

Initially I was doing $offset = $offset % $frequency but that prevented setting the offset before the frequency, and it's useful to return a metronome with a pre-seeded offset so I got rid of that : function newMetronomeForThisServer() { return metronome->setOffsetFromSeed(php_uname('n')); }.

This code now has no effect except preventing the use case I actually want (setting offset before frequency). I removed it locally but neglected to update the change.

54

(It's slightly biased, but okay for the current use cases of "profile pictures" and "make servers kind of not call services all at the same time".)

67

Yeah -- digestToRange() also uses a simple % and is biased (although I think the low-order bits of sha1 are not particularly weak?) and on top of everything else this code also doesn't do the right thing with leap seconds. This is definitely aimed at being "better than random jitter", not "ready to launch into orbit".

epriestley updated this revision to Diff 47992.Feb 5 2019, 8:17 PM
  • Remove spurious "getFrequency()" call.
This revision was automatically updated to reflect the committed changes.