----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31821/#review75637 -----------------------------------------------------------
Ship it! src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java <https://reviews.apache.org/r/31821/#comment122841> Can you leave a TODO(wfarner) to reuse existing modules here? The DRY violation here is only going to get worse until we do. src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java <https://reviews.apache.org/r/31821/#comment122842> Careful about using 'priority' here, as it can be confused for the field within `TaskConfig`. With that in mind, you might move this doc to `PreemptorImpl`, as it's more of an implementation definition than interface. src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java <https://reviews.apache.org/r/31821/#comment122843> This makes for an interesting opportunity - this bit of data could be generalized to apply to any scheduling (whether or not preemption is required). Nothing to do with this now, but you could imagine abstracting all scheduling decisions to fit behind this facade at some point. src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java <https://reviews.apache.org/r/31821/#comment122844> Nothing forces the backing Set to be immutable, consider changing the constructor to accept `ImmutableSet` to bolster immutability. src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java <https://reviews.apache.org/r/31821/#comment122845> s/final // src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java <https://reviews.apache.org/r/31821/#comment122846> Aha! I knew i missed this in a previous patch. We no longer have tri-state logic here (we wither return Optional.absent or a non-empty Set), so you can simplify this comment. src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java <https://reviews.apache.org/r/31821/#comment122847> Not yours, but it seems odd that we would call a function and internally suppress its behavior based on configuration. Seems like the caller should avoid the call if the behavior is not desired. If you agree, please TODO. src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java <https://reviews.apache.org/r/31821/#comment122848> s/static // Also not yours, but wording is confusing here. Specifically, when i read "the provided task" i think of the `pendingTask` parameter, but it's actually the opposite. How about: ``` Creates a filter that will find tasks that the provided {@code pendingTask} may preempt. ``` src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImpl.java <https://reviews.apache.org/r/31821/#comment122849> I'm not sure if this is the correct spot, but consider leaving a TODO to validate consistency of the (possibly weakly-consistent) preemption proposal. src/test/java/org/apache/aurora/scheduler/async/preemptor/TestUtil.java <https://reviews.apache.org/r/31821/#comment122850> I'm highly skeptical of the shelf life of test fixtures like this. Here we have 6 primitive parameters, making it very easy to accidentally swap them at the call site. It's slightly different when the utility is within a unit test, but sharing only accelerates towards an unmaintainable state. I suggest you either minimally duplicate these helpers in the two unit test classes, or trim down the parameters to something more universal like: ``` makeTask(IJobKey job, String id) ``` and make the caller mutate the result for any additional customization. Consider this as a TODO for now, so we can avoid further modification of the test cases. However, in this diff please correct the line wrap style on the arguments. - Bill Farner On March 7, 2015, 1:36 a.m., Maxim Khutornenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31821/ > ----------------------------------------------------------- > > (Updated March 7, 2015, 1:36 a.m.) > > > Review request for Aurora, Bill Farner and Zameer Manji. > > > Bugs: AURORA-1158 > https://issues.apache.org/jira/browse/AURORA-1158 > > > Repository: aurora > > > Description > ------- > > Extracting PreemptorSlotFinder to be reused for slot validation in later > stages. The changes are very minimal and mostly around metric handling and > test code. > > Also added missing test coverage. > > > Diffs > ----- > > src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java > 701b9052696337766cb233c865cb9fbb4907071e > src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java > e093ca54521ffb9399bb97ce60f510331af70853 > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/async/preemptor/Preemptor.java > bddb9647493b3e7a58c40d4b477a06161c1388a2 > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImpl.java > ae56d1e09322869eedd7a27586cd6f96edd64e0a > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorMetrics.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java > 85b3874a36ed07c684f26da172952c932cff707a > src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java > 58733bdc4dd6de29ccead5cb0a267286e8dc0656 > src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java > 891cc098cca99e84ba014b7131106ceb0b429b5f > > src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java > 83680769611878886da04e1794b321aa1986e678 > > src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModuleTest.java > 020b67187a18bba64d9b562c3a6c0969fc85d469 > > src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java > PRE-CREATION > src/test/java/org/apache/aurora/scheduler/async/preemptor/TestUtil.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/31821/diff/ > > > Testing > ------- > > ./gradlew -Pq build > > > Thanks, > > Maxim Khutornenko > >
