----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64954/#review194775 -----------------------------------------------------------
Reviewer notes commons/src/main/java/org/apache/aurora/common/testing/easymock/EasyMockTest.java Lines 48-49 (patched) <https://reviews.apache.org/r/64954/#comment273754> Unnecessary for the scheduling refactor, but scratching an itch. src/main/java/org/apache/aurora/scheduler/TierManager.java Line 105 (original), 106 (patched) <https://reviews.apache.org/r/64954/#comment273755> This was a potential bug - `TierManager` relied on the caller to inject the default tier, but would return `null` here if an unknown tier was requested. src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettings.java Lines 55-58 (patched) <https://reviews.apache.org/r/64954/#comment273756> This logic was previously duplicated. src/main/java/org/apache/aurora/scheduler/offers/HostOffers.java Lines 161 (patched) <https://reviews.apache.org/r/64954/#comment273757> No logic change, just simplified. src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImpl.java Lines 153-156 (original), 145-148 (patched) <https://reviews.apache.org/r/64954/#comment273758> Captures the tri-state logic to represent reservations. I believe the factory and accessor methods sufficiently self-document. src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImpl.java Lines 175 (patched) <https://reviews.apache.org/r/64954/#comment273761> ReviewBoard's diff algorithm is a bit aggressive here, making it difficult to comprehend the changes as rendered. Just wanted to call out that this is split into a helper method. src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImpl.java Line 240 (original), 225 (patched) <https://reviews.apache.org/r/64954/#comment273762> This is the primary objective of the change - we now have an isolated body of code that collects scheduling matches. src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImpl.java Lines 244-250 (original) <https://reviews.apache.org/r/64954/#comment273763> Rather than "first try to match all tasks against reservations", the patch treats a reservation as an alternate way to acquire a `SchedulingMatch`. src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java Line 34 (original), 34 (patched) <https://reviews.apache.org/r/64954/#comment273764> `Iterable` type forced an unnecessary `ImmutableSet.copyOf()` in the implementation. No good reason to keep `Iterable`. src/main/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImpl.java Lines 132-138 (original) <https://reviews.apache.org/r/64954/#comment273765> This check failure becomes a failure in `Iterables.getOnlyElement()` on :147. src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java Line 50 (original), 51 (patched) <https://reviews.apache.org/r/64954/#comment273767> Allow extra hurdles to use `size()`, `isEmpty()`, and `stream()`. src/main/java/org/apache/aurora/scheduler/updater/UpdateAgentReserver.java Line 67 (original), 64 (patched) <https://reviews.apache.org/r/64954/#comment273768> Slimmed this interface, the extra return detail was not used in practice. src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java Line 122 (original) <https://reviews.apache.org/r/64954/#comment273770> Simplify with mock -> fake. I can't think of a good reason to mock `TierManager`, as it's just a map with a default. - Bill Farner On Jan. 4, 2018, 11:08 a.m., Bill Farner wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64954/ > ----------------------------------------------------------- > > (Updated Jan. 4, 2018, 11:08 a.m.) > > > Review request for Aurora, Daniel Knightly and Jordan Ly. > > > Repository: aurora > > > Description > ------- > > This patch sets the stage for performing the bulk of scheduling work in a > separate call path, without holding the write lock. > > Also included is a mechanical refactor pushing the `revocable` flag into > `ResourceRequest` (which was ~always needed as a sibling parameter). > > > Diffs > ----- > > > commons/src/main/java/org/apache/aurora/common/testing/easymock/EasyMockTest.java > c5500b3658437d0a940f953957c880559f5f1b61 > src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java > f0dacd4e90bc142df8b6489ffb33d7265ec03706 > src/main/java/org/apache/aurora/scheduler/TierManager.java > c6ad2b1c48673ca2c14ddd308684d81ce536beca > src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java > 22d5a642687c1ddc91071c27abda7d61726b28b9 > > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettings.java > 5c987fd051728486172c8afd34219e86d56f00d5 > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java > bd415909ac1abdb670fde57ead0e391de3e5238b > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > cb288bb12d577330888c6a66ddd2baaff4b71e8e > src/main/java/org/apache/aurora/scheduler/offers/HostOffers.java > a01c0a87856a63b1734b4ea336bbbf5fa74b8636 > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java > e90de3eec43a3df9ff019830e927ecff195d1747 > src/main/java/org/apache/aurora/scheduler/offers/OfferManagerImpl.java > 8e806b705d8bee4381ed8afd792d777420405195 > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictim.java > 69b686639de1eb7d896096e2fadb357fe9ef3141 > > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java > cf6d348d645d78ab77fcd305307df8a94fea114c > src/main/java/org/apache/aurora/scheduler/resources/ResourceManager.java > d093753b715ffc9ff37448f0266903d6778af024 > src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssigner.java > 87619b55dae0c49029d4f804a52fdcbf9f16a50a > src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImpl.java > 54bd1770274dcedecacccba697aee65b8dd48347 > src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java > 3c38f953ed830a2e3627705aa6493e9b37dae155 > src/main/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImpl.java > cff4ab196451fc846827cfc1f48897e645ac3d5d > src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java > 1219215d610e0dbe1f4e78bb8ea57504de0fa329 > > src/main/java/org/apache/aurora/scheduler/storage/durability/WriteRecorder.java > dea8e690d8325596c70f31212029ddcfdf8e4451 > src/main/java/org/apache/aurora/scheduler/updater/UpdateAgentReserver.java > b6909a692b7ea590768ecead08b59277b5577866 > src/test/java/org/apache/aurora/scheduler/TierManagerTest.java > 82e40d509d84c37a19b6a9ef942283d908833840 > > src/test/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilterTest.java > 64d7a44eaeb627daf7b7bee75ac0be19594a9b25 > > src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java > 0de90d70afa1bfa00c93875a325b8a3a7ed7b03d > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java > c27a662a2a03ee15098ff66229f86da831177a52 > src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java > 1e9532bef39591fcc61a2b41a04f2290960d8906 > > src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java > 348386e938ce910c73f1e2e6e2b39f3654a85e7e > > src/test/java/org/apache/aurora/scheduler/scheduling/FirstFitOfferSelectorTest.java > 5e2fdcb646a3362043f171bfc201b83d76524d78 > > src/test/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImplTest.java > 78e12691be0bf20e868ad202aaa306ef29bf5971 > > src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java > ecf2987bf9fa124a4de3a93ffb20b4a807d79b3e > > src/test/java/org/apache/aurora/scheduler/updater/NullAgentReserverTest.java > d8563b86747a7887be9262a621fd761fea832b8c > > src/test/java/org/apache/aurora/scheduler/updater/UpdateAgentReserverImplTest.java > 7f17be0427e3baf12aaa1d176414f7009baeea94 > > > Diff: https://reviews.apache.org/r/64954/diff/1/ > > > Testing > ------- > > > Thanks, > > Bill Farner > >
