> On March 7, 2015, 5:30 p.m., Bill Farner wrote: > > src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java, line 97 > > <https://reviews.apache.org/r/31821/diff/1/?file=888146#file888146line97> > > > > Can you leave a TODO(wfarner) to reuse existing modules here? The DRY > > violation here is only going to get worse until we do.
I share your concern. Could not find a clean way of doing it though. Added a TODO. > On March 7, 2015, 5:30 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java, > > line 65 > > <https://reviews.apache.org/r/31821/diff/1/?file=888148#file888148line65> > > > > 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. This is a reprhrased original content but I see your point. Even though that field is used in comparison it does not define the policy alone. I do feel this is the right place for it though as PreemptorImpl will have a slightly broader meaning when async background worker support is implemented. Rephrased. > On March 7, 2015, 5:30 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java, > > line 90 > > <https://reviews.apache.org/r/31821/diff/1/?file=888148#file888148line90> > > > > Nothing forces the backing Set to be immutable, consider changing the > > constructor to accept `ImmutableSet` to bolster immutability. > > Zameer Manji wrote: > Alternatively, the constructor could just store an immutable copy which > is a pattern that is done elsewhere. Good idea. Done. > On March 7, 2015, 5:30 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java, > > line 251 > > <https://reviews.apache.org/r/31821/diff/1/?file=888148#file888148line251> > > > > s/final // It's actually needed as it's accessed from Predicate. > On March 7, 2015, 5:30 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java, > > line 268 > > <https://reviews.apache.org/r/31821/diff/1/?file=888148#file888148line268> > > > > 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. Done. > On March 7, 2015, 5:30 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java, > > line 326 > > <https://reviews.apache.org/r/31821/diff/1/?file=888148#file888148line326> > > > > 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. Not sure I share your concern. This is a predicate intended to answer a simple question, which it does quite well. I doubt the alternative would gain us anything here. > On March 7, 2015, 5:30 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java, > > line 344 > > <https://reviews.apache.org/r/31821/diff/1/?file=888148#file888148line344> > > > > 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. > > ``` Done. > On March 7, 2015, 5:30 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImpl.java, > > line 369 > > <https://reviews.apache.org/r/31821/diff/1/?file=888150#file888150line369> > > > > 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. Yeah, I was going to address it in the next step. Updated the "read-only" TODO. > On March 7, 2015, 5:30 p.m., Bill Farner wrote: > > src/test/java/org/apache/aurora/scheduler/async/preemptor/TestUtil.java, > > line 34 > > <https://reviews.apache.org/r/31821/diff/1/?file=888158#file888158line34> > > > > 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. > > Zameer Manji wrote: > +1 to Bill's suggestion of having `makeTask(IJobKey job, String id)`. I > think having a fixture with this many options will only lead to complexity > and sadness if the tests increase in scope. Agree. Dropped. - Maxim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31821/#review75637 ----------------------------------------------------------- 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 > >
