----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51993/#review151341 -----------------------------------------------------------
The general approach looks reasonable to me. Please address the comments below and also, please add test coverage for the changes to the `PendingTasks` servlet as well as for the new method in `NearestFit`. src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java (line 126) <https://reviews.apache.org/r/51993/#comment219680> Add a newline as a separator between the method description and the parameter list (for consistency with other javadoc throughout the code) src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java (lines 129 - 130) <https://reviews.apache.org/r/51993/#comment219675> Maybe just have this return `void` so it's clear that the `TaskGroup`'s will be modified in place? src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java (lines 129 - 130) <https://reviews.apache.org/r/51993/#comment219676> This fits on one line. src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java (line 131) <https://reviews.apache.org/r/51993/#comment219677> Formatting should be: // Iterating over ... (i.e. space after the slashes, sentence capitalization). That said, this comment is of questionable value, so I'd be fine with dropping it entirely... src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java (lines 135 - 137) <https://reviews.apache.org/r/51993/#comment219678> Indentation is off here, should be +4 chars for continuation indents. That said, even cleaner would just be to inline this directly into the `setReason` call below. src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java (line 43) <https://reviews.apache.org/r/51993/#comment219679> Making this method public is what's causing Jackson to fail to serialize this and necessitating your use of Gson in the PendingTasks servlet. You can avoid that be annotating this method with `@JsonIgnore` (but make sure you use the annotation from `org.codehaus...` not `com.fasterxml...` With that done, the return from `PendingTasks#getOffers` should be able to revert back to `Response.ok(taskGroups.getGroups()).build()` (you'll still need to call `addPendingReasong` beforehand of course). - Joshua Cohen On Oct. 4, 2016, 3:20 a.m., Pradyumna Kaushik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51993/ > ----------------------------------------------------------- > > (Updated Oct. 4, 2016, 3:20 a.m.) > > > Review request for Aurora and Maxim Khutornenko. > > > Repository: aurora > > > Description > ------- > > Added the 'reason' to the /pendingTasks endpoint > > > Diffs > ----- > > src/main/java/org/apache/aurora/scheduler/http/PendingTasks.java > c80e0c8adf80e12082a6952ae79b7d9cc960c5b6 > src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java > f783e7ff220573915524a1efc27141193d19fa6c > src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroup.java > 5d319557057e27fd5fc6d3e553e9ca9139399c50 > > Diff: https://reviews.apache.org/r/51993/diff/ > > > Testing > ------- > > ./build-support/jenkins/build.sh > ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > > Thanks, > > Pradyumna Kaushik > >
