> On March 28, 2016, 9:36 a.m., Maxim Khutornenko wrote: > > src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java, > > line 131 > > <https://reviews.apache.org/r/45222/diff/1/?file=1315322#file1315322line131> > > > > Any reason to allow more than "once" (default) here? Same in other > > tests. > > Amol Deshmukh wrote: > The actual cardinality of the call ``tierManager.getTier(lowPriority)`` > here is 3: > 1. Once in the ``victimToResources`` filter (existing behavior) > 2. Twice in > ``o.a.a.s.preemptor.PreemptionVictimFilter.PreemptionVictimFilterImpl#filterPreemptionVictims``, > due to the lazy evaluation of the ``FluentIterable preemptableTasks``: > a. Once via ``preemptableTasks.isEmpty()`` > b. Once via ``resourceOrder.immutableSortedCopy(preemptableTasks)`` > > Now that I think about it, it is just as easily possible to have the > FluentIterable evaluated once by making the ``immutableSortedCopy`` before > checking for emptiness. I will repost an updated review.
Ok, so while that optimization does cut down the number of times the filter operation is performed, there is still some indeterminism introduced due to the use of ``victimToResources`` transform in ``resourceOrder`` for tests involving more than one eligible victim. Those cases, I believe, will be best served by the use of ``atLeastOnce()`` instead of the exact cardinality discovered through testing. - Amol ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45222/#review125671 ----------------------------------------------------------- On March 25, 2016, 5:08 p.m., Amol Deshmukh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45222/ > ----------------------------------------------------------- > > (Updated March 25, 2016, 5:08 p.m.) > > > Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner. > > > Bugs: AURORA-1616 > https://issues.apache.org/jira/browse/AURORA-1616 > > > Repository: aurora > > > Description > ------- > > Introduce "preemptible" flag in TierInfo with backward compatible support for > "production" flag in TaskConfig. > > # Summary of changes > - TierInfo extended to manage a new property: "preemptible" > - If TaskConfig does not specify a tier, ``TierManager.getTier(..)`` falls > back to selecting a non-revocable tier matching ``TaskConfig.isProduction()``. > - Removed ``TierInfo.DEFAULT`` and replaced its in test/benchmark code by > references to ``TaskTestUtil.DEV_TIER``. > - Eager validation of tier specified in TaskConfig in > ``ConfigurationManager.validateAndPopulate(..)`` > > # Note on backward incompatible change > TaskConfigs with a tier name not matching any tiers defined in ``tiers.json`` > were silently assigned a default tier. With this change, attempting to > schedule tasks that specify non-existent tier names will result in an error > (see ``ConfigurationManager.validateAndPopulate(ITaskConfig config)``). > > > Diffs > ----- > > src/jmh/java/org/apache/aurora/benchmark/Offers.java > 055a2ffcb13c643a3086343e3fbf71545c5fb0a6 > src/main/java/org/apache/aurora/scheduler/TierInfo.java > b4611b9cf99ca236cd1ea4610d01c0d293bf2e87 > src/main/java/org/apache/aurora/scheduler/TierManager.java > 480c4853a6c87dd63a9810ae013e5cfacb29336d > src/main/java/org/apache/aurora/scheduler/app/AppModule.java > 97d87ff1b789625f2c07baf7a74a076f07742596 > src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java > 9cf8002e932d562e93fb8d17b4c56f564eb54ed5 > > src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java > b3b8ccf868c2a2f18f720a837e90d763072dd3eb > src/main/java/org/apache/aurora/scheduler/mesos/TestExecutorSettings.java > 16fa2d35cdf7ecdf82dd1ec7739e685ec1ff1aa2 > > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java > 7f84e90774193b0d31adb7dafcab0a249167cdba > src/main/resources/org/apache/aurora/scheduler/tiers.json > 18701058076bedc5d1b667e2b97ad09ce84a9bb9 > src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java > 39096af816864ada32a9c07958975740e805f6b0 > src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java > 52113b80d91ecaf0cc2aeaad77e5fbc0ea4d1216 > src/test/java/org/apache/aurora/scheduler/ResourcesTest.java > 4fe8c518c580418078275b8056a5c195a765681e > src/test/java/org/apache/aurora/scheduler/TierManagerTest.java > a662100ba726cff0e47b4f9650753db9cecdef51 > src/test/java/org/apache/aurora/scheduler/TierModuleTest.java > e0601c83486671596e412f022ffae78b01c81c9d > > src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java > 1a520b3cb035a9afc25406d2f313c9f861eee4d6 > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java > 5103bd0f43d53079976b0f1596e299f2d91aa860 > > src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java > b6f5e4632ac1e028fdf93da1735463373e2d2788 > src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java > b00add0b2fd4277e196505fffba4440e2e94207e > src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java > b1c71a6f1847d205c378d0b5a7269ea9d1165be5 > > Diff: https://reviews.apache.org/r/45222/diff/ > > > Testing > ------- > > # End to end tests > ``` > $ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > ... > *** OK (All tests passed) *** > ``` > > # Jenkins build.sh > ``` > $ ./build-support/jenkins/build.sh > ... > SUCCESS > ``` > > # Local Scheduler > ``` > $ ./gradlew run > ... > I0325 16:00:55.374 [pool-9-thread-1, FakeMaster:139] All offers consumed, > suppressing offer cycle. > I0325 16:01:00.371 [pool-9-thread-1, FakeMaster:139] All offers consumed, > suppressing offer cycle. > ``` > > # Attempting to schedule job with invalid tier-name > ``` > vagrant@aurora:~/workspace$ aurora job create devcluster/vagrant/devel/test > job.aurora > INFO] Creating job test > Job creation failed due to error: > Invalid tier 'badtier' in TaskConfig. > ``` > > > Thanks, > > Amol Deshmukh > >
