Add a "metronome" for spreading service call load
ClosedPublic
Actions

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

Details

Reviewers

amckinley

Maniphest Tasks

T13244: 2019 Week 6 Bonus Content

Commits

rP4675306615da: Add a "metronome" for spreading service call load

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

Branch

metronome1

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 21835
Build 29798: Run Core Tests
Build 29797: arc lint + arc unit

Event Timeline

epriestley created this revision.Feb 5 2019, 1:26 AM

Harbormaster completed remote builds in B21817: Diff 47973.Feb 5 2019, 1:28 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!
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".