> On Nov. 20, 2017, 7:55 p.m., Jordan Ly wrote: > > src/main/java/org/apache/aurora/scheduler/filter/AttributeAggregate.java > > Line 52 (original) > > <https://reviews.apache.org/r/63973/diff/1/?file=1898144#file1898144line52> > > > > Note for reviewers: this was causing some tests to fail due to my > > refactors not calling the initialization. > > > > I removed this optimization since it seemed to be geared toward the > > internal DB implementation we just removed. The benchmarks look alright, > > but I may be overlooking something from the original issue. > > > > I was looking at this patch as context: > > https://reviews.apache.org/r/53918/
Unfortunate if not covered in tests, but this appears to be necessary for correctness when multiple tasks are scheduled per scheduling round. This seems to be the review for meaningful context: https://reviews.apache.org/r/51929/ When a task is successfully scheduled in a scheduling round, the AttributeAggregate is updated so that it remains correct in a subsequent attempt within the same round. - Bill ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63973/#review191561 ----------------------------------------------------------- On Nov. 20, 2017, 7:40 p.m., Jordan Ly wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63973/ > ----------------------------------------------------------- > > (Updated Nov. 20, 2017, 7:40 p.m.) > > > Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, > Stephan Erb, and Bill Farner. > > > Repository: aurora > > > Description > ------- > > This is a rather large diff. Posting without tests first to get initial > impressions/make the review more digestible, but tests are soon to come. > > Major portions of the refactor: > > * Allows injection of custom `OfferManager` via `OfferManagerModule` > * Refactor `OfferManager` to do filtering of offers (added `getMatching` and > `getAllMatching` methods) as opposed to TaskAssigner > * Refactor `TaskAssigner`, allow for injection of custom "scoring" class > through `OfferRanker` interface > > And some minor things as well: > > * Moved `TaskAssignerImpl`, `TaskSchedulerImpl`, and `HostOffers` into their > own upper-level classes > * Moved `TaskAssigner` to the `scheduling` package and out of the `state` > package > * Renamed some methods in `OfferManager` to avoid code stutter > * Renaming of some classes (e.g. `FirstFitTaskAssigner` -> `TaskAssignerImpl`) > * And a slew of others > > Overall, the goal of this refactor is to allow for the easy injection of > custom offer "scoring" modules via the `OfferRanker` interface. Callers will > not have to replace the entire `TaskAssigner` module, but instead just > provide a function that chooses from a list of offers that should already fit > the task. > > Going through this review, I would start with the > `maybeAssign`/`launchUsingOffer` methods in `TaskAssigner`. Then, I would > look at `getMatching`/`getAllMatching` in `HostOffers`. > > > Diffs > ----- > > docs/reference/scheduler-configuration.md > d17541f9458650240983276b69f749a854057aa8 > src/main/java/org/apache/aurora/scheduler/app/AppModule.java > 3204cca5446d0f798ace622d334473bf6db13733 > src/main/java/org/apache/aurora/scheduler/config/CliOptions.java > d4537e386af53ec9bff661ed23d61a5bd4893edf > src/main/java/org/apache/aurora/scheduler/filter/AttributeAggregate.java > 60f141d34d862f200ae1346a38a71bba57551059 > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java > 36608a9f027c95723c31f9915852112beb367223 > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java > df51d4cf4893899613683603ab4aa9aefa88faa6 > src/main/java/org/apache/aurora/scheduler/http/Offers.java > f22ca6e5402d8ec7241376de14a9051ab0cfbfc6 > src/main/java/org/apache/aurora/scheduler/mesos/MesosCallbackHandler.java > fd5874d51cfbca1b98d15623a9c845d208d43b1f > src/main/java/org/apache/aurora/scheduler/offers/HostOffers.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java > 96b0f46e69255944fa0f00621db315e22d68515c > src/main/java/org/apache/aurora/scheduler/offers/OfferManagerImpl.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/offers/OfferManagerModule.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java > 57fc1a18f1b82decc780ba0069ab4dd0c125f77d > src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java > 4a6ea8dac97ed2f5df47e4d16fa65a3b516988bd > > src/main/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessor.java > 766d3b24fd64de96b82d3dd2a17f7a80331889d8 > src/main/java/org/apache/aurora/scheduler/preemptor/Preemptor.java > 82a0ff67e9f70fed305f8f1bf38e3cf7955b9ce7 > > src/main/java/org/apache/aurora/scheduler/scheduling/FirstFitOfferRanker.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/scheduling/OfferRanker.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/scheduling/SchedulingModule.java > 0796712a00e3cba595ceb049833933b9a70c2f9a > src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssigner.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImpl.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImplModule.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java > 0002b0ceb429a404ac9dd0ae7e5ea9f6362fa100 > src/main/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImpl.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/state/FirstFitTaskAssignerModule.java > dc244eeaa0d292a06d8750cb340fb379701ad49f > src/main/java/org/apache/aurora/scheduler/state/StateModule.java > c03fff11ea3a4086f9daaa8b07315006c1b481e4 > src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java > cdd0d15872248b19d5eb52d25537f2f863fd5c77 > src/main/java/org/apache/aurora/scheduler/stats/AsyncStatsModule.java > 6033c01477bec7a491128e3089ffc1b1c3f1c150 > > > Diff: https://reviews.apache.org/r/63973/diff/1/ > > > Testing > ------- > > Tests coming soon, but they should be relatively close to what we have now on > master since we don't really add/remove features but just move them around. > > Ran some benchmarks as well to ensure performance parity: > > ``` > MASTER > Benchmark > Mode Cnt Score Error Units > SchedulingBenchmarks.ClusterFullUtilizationBenchmark.runBenchmark > thrpt 10 113910.922 ± 26263.903 ops/s > SchedulingBenchmarks.FillClusterBenchmark.runBenchmark > thrpt 10 263.043 ± 299.686 ops/s > SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark > thrpt 10 12353.669 ± 433.182 ops/s > SchedulingBenchmarks.LimitConstraintMismatchSchedulingBenchmark.runBenchmark > thrpt 10 893.263 ± 54.882 ops/s > SchedulingBenchmarks.ValueConstraintMismatchSchedulingBenchmark.runBenchmark > thrpt 10 11478.420 ± 1068.819 ops/s > ``` > and > ``` > MY PATCH > Benchmark > Mode Cnt Score Error Units > SchedulingBenchmarks.ClusterFullUtilizationBenchmark.runBenchmark > thrpt 10 124041.966 ± 33331.424 ops/s > SchedulingBenchmarks.FillClusterBenchmark.runBenchmark > thrpt 10 1030.434 ± 1223.539 ops/s > SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark > thrpt 10 12305.154 ± 375.814 ops/s > SchedulingBenchmarks.LimitConstraintMismatchSchedulingBenchmark.runBenchmark > thrpt 10 1080.267 ± 74.635 ops/s > SchedulingBenchmarks.ValueConstraintMismatchSchedulingBenchmark.runBenchmark > thrpt 10 11604.642 ± 586.248 ops/s > ``` > > Not exactly sure why `FillClusterBenchmark` increased so dramatically -- I > will look into that while testing as well. > > > Thanks, > > Jordan Ly > >
