Reduce garbage-level of Drydock Allocator implementation
ClosedPublic
Actions

Authored by epriestley on Sep 15 2015, 6:09 PM.

Details

Reviewers

hach-que
chad

Maniphest Tasks

T9253: Implement a Drydock blueprint for leasing hosts from a static, pre-built Almanac pool

Commits

Restricted Diffusion Commit
rPbb28f94f9b64: Reduce garbage-level of Drydock Allocator implementation

Summary

Ref T9253. The Drydock allocator is very pseudocodey right now. Particularly, it was written before Blueprints were concrete.

Reorganize it to make its responsibilities and error handling behaviors more clear.

In particular, the Allocator does not manage locks. It's primarily trying to reject allocations which can not possibly work. Blueprints are responsible for locks. See some discussion in D10304.

NOTE: This code probably doesn't work as written, see future diffs.

Test Plan

See future diffs.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley updated this revision to Diff 34105.Sep 15 2015, 6:09 PM

epriestley retitled this revision from to Reduce garbage-level of Drydock Allocator implementation.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added reviewers: chad, hach-que.

epriestley added a task: T9253: Implement a Drydock blueprint for leasing hosts from a static, pre-built Almanac pool.

epriestley added inline comments.Sep 15 2015, 6:13 PM

src/applications/drydock/worker/DrydockAllocatorWorker.php
119–120	There's one major change structural change happening here, and this is the best example of it: previously, we generally didn't involve Blueprints themselves in any of this filtering at all. A long time ago, Blueprints weren't stored (this was a gross simplifying assumption in v-5 of the code). They were turned into real objects in D7638 but never really threaded through everything fully. This resulted in silliness like this: $blueprint_implementation->canAllocateMoreResources($pool); ...approximately, on line 136. This is nonsense: the blueprint implementation can not possibly answer that question and `$pool` is meaningless. The new code starts with concrete Blueprints and just does a bunch of layers of "does this make sense?" checks, all of which are independently reasonable.