----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65233/#review195838 -----------------------------------------------------------
Ship it! All nits, nice patch! src/main/java/org/apache/aurora/scheduler/offers/HostOffers.java Line 66 (original), 65 (patched) <https://reviews.apache.org/r/65233/#comment275166> Inject `OfferSet` rather than `OfferSettings`. src/main/java/org/apache/aurora/scheduler/offers/HostOffers.java Lines 180-182 (original), 177-180 (patched) <https://reviews.apache.org/r/65233/#comment275168> Revert. If there is no downside to chaining, i think it's much more readable. src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java Line 96 (original), 96 (patched) <https://reviews.apache.org/r/65233/#comment275167> Revert noop src/main/java/org/apache/aurora/scheduler/offers/OfferManagerModule.java Line 91 (original), 91 (patched) <https://reviews.apache.org/r/65233/#comment275169> `offer_set_module` singular -> no custom splitter, no `List`. src/main/java/org/apache/aurora/scheduler/offers/OfferSet.java Lines 9 (patched) <https://reviews.apache.org/r/65233/#comment275170> s/collection/set/. That distinction makes me comfortable leaving most of these methods without docs. src/main/java/org/apache/aurora/scheduler/offers/OfferSet.java Lines 12 (patched) <https://reviews.apache.org/r/65233/#comment275171> Remove VisibleForTesting src/main/java/org/apache/aurora/scheduler/offers/OfferSet.java Lines 23 (patched) <https://reviews.apache.org/r/65233/#comment275175> Up to you, but i'm not seeing a good reason to return `Iterable` over `Stream` here and below. It has the upside of no extra hoops for filtering and no need to protect from modification. src/main/java/org/apache/aurora/scheduler/offers/OfferSet.java Lines 23-30 (patched) <https://reviews.apache.org/r/65233/#comment275176> How about this renaming: `getAll()` -> `values()` (aligning with Collection terminology) `getAll(...)` -> `getOrdered(...)` src/main/java/org/apache/aurora/scheduler/offers/OfferSet.java Lines 26 (patched) <https://reviews.apache.org/r/65233/#comment275177> Please expand this doc a bit. What does context mean? src/main/java/org/apache/aurora/scheduler/offers/OfferSetImpl.java Lines 78 (patched) <https://reviews.apache.org/r/65233/#comment275172> Omit comment, non-informative src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java Lines 72 (patched) <https://reviews.apache.org/r/65233/#comment275173> remove src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImpl.java Line 241 (original), 239 (patched) <https://reviews.apache.org/r/65233/#comment275174> Remove comment src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java Line 192 (original) <https://reviews.apache.org/r/65233/#comment275178> If i'm reading correctly, this test is still relevant with the default `OfferSetImpl`. Any reason we should remove it? - Bill Farner On Jan. 19, 2018, 10:11 a.m., Jordan Ly wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65233/ > ----------------------------------------------------------- > > (Updated Jan. 19, 2018, 10:11 a.m.) > > > Review request for Aurora, David McLaughlin, Stephan Erb, and Bill Farner. > > > Repository: aurora > > > Description > ------- > > The goal of this patch is to provide a more reasonable abstraction for custom > scheduling. > > Currently, we have `OfferSelector`, `OfferOrder`, and recently I proposed > `FilterableOfferCollection` (https://reviews.apache.org/r/65225/). These were > all created in order to provide more customization within the scheduling > loop. However, they suffer from being leaky and too disparate. This patch > hopes to combine all of those components into a single `OfferSet` which can > be injected and used by HostOffers. This interface allows for custom > scheduling logic to be co-located with custom data structures for `offers` as > opposed to being passed around as different components. > > The following options will be removed from the last 0.19 to now: > ``` > -offer_order > -offer_order_modules > ``` > > > Diffs > ----- > > src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java > f44cb61f5d1805aeb0f98135453dbc82f3bc9ad4 > src/main/java/org/apache/aurora/scheduler/config/CliOptions.java > e4e535824adb86c98c6173a20f97a9a90b08483a > src/main/java/org/apache/aurora/scheduler/offers/HostOffers.java > 2ea7a01085b87c8ed6765537a8005e2349784ab0 > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java > 8f9e33d81be9087821784a8a08079a1736d9cb63 > src/main/java/org/apache/aurora/scheduler/offers/OfferManagerModule.java > de16c14c887d4225e0629b1580eb5e740798f1f7 > src/main/java/org/apache/aurora/scheduler/offers/OfferOrder.java > 7c99c309e0e97519719905f725bb9f6a59d2a7f5 > src/main/java/org/apache/aurora/scheduler/offers/OfferOrderBuilder.java > 1260ef19506acb8e8a937d4fd7b7152361bd3c40 > src/main/java/org/apache/aurora/scheduler/offers/OfferSet.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/offers/OfferSetImpl.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java > 1e36b2c3094e99f05aa4a4f098a48df2293b4320 > > src/main/java/org/apache/aurora/scheduler/scheduling/FirstFitOfferSelector.java > 5f4f452cb133e2be73f881700a54ba4c8e60a35a > > src/main/java/org/apache/aurora/scheduler/scheduling/FirstFitOfferSelectorModule.java > 4d36487f430483444314a8cefd133e7c766075de > src/main/java/org/apache/aurora/scheduler/scheduling/OfferSelector.java > 6f5d55c5a586ac923b5f375d1935ec2049673ec0 > src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImpl.java > ec416cc12b2fb4a11a273c77dbe72b79f96fa7e1 > > src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImplModule.java > d101a497896ba3b8ca7984a63f861494cbddbb35 > src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java > c84fad3b615c49ca7527ae3f7fa116aa8f90ff0c > src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java > 28224a5c587b235ccc86a286e796eeca66929232 > > src/test/java/org/apache/aurora/scheduler/scheduling/FirstFitOfferSelectorTest.java > e6b2b7465fb4fd432cda50d1e714dfc587764689 > > src/test/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImplTest.java > f84941edd96d974a6c2d7e087324bc8fbbac3b0a > > > Diff: https://reviews.apache.org/r/65233/diff/2/ > > > Testing > ------- > > `./gradlew test` > `./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh` > > Testing injection of a custom `OfferSet` onto a test cluster. > > > Thanks, > > Jordan Ly > >
