----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42126/#review113768 -----------------------------------------------------------
Nice job! Most comments here are nits to align code style/conventions. I've mostly left `OfferAllocation.java` alone in this pass, but you should consider applying the spirit of these comments there (specifically immutability, testing public interface). Also, the name `OfferAllocation` doesn't sound quite right. How about `AcceptedOffer`? I don't have a strong opinion here, names are hard and highly subjective :-) src/main/java/org/apache/aurora/scheduler/OfferAllocation.java (line 188) <https://reviews.apache.org/r/42126/#comment174594> Consider handling `revocable` with a filter on the result of this function rather than an argument since only one caller passes something other than `false`. src/main/java/org/apache/aurora/scheduler/ResourceSlot.java (line 100) <https://reviews.apache.org/r/42126/#comment174578> +1 here and below, consider doing that in this patch or an immediate follow-up if you'd like to minimize the patch delta. Your call. src/main/java/org/apache/aurora/scheduler/Resources.java (line 157) <https://reviews.apache.org/r/42126/#comment174579> Reminder to remove this src/main/java/org/apache/aurora/scheduler/Resources.java (line 169) <https://reviews.apache.org/r/42126/#comment174580> Always prefer immutable when it makes sense. For this function, it's a matter of changing to something like: ``` ImmutableList.Builder<Range> ranges = ImmutableList.builder(); ... ranges.addAll(Iterables.concat(r.getRanges().getRangeList().iterator())); ... return ranges.build(); `` src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 105) <https://reviews.apache.org/r/42126/#comment174576> Nit: our style is to only use the `final` keyword when necessary, or for class fields (for reference immutability). s/final // src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 106) <https://reviews.apache.org/r/42126/#comment174577> This block of code doesn't do much explicit validation of fields within the protobuf, so IMHO you can safely remove this line. src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 120) <https://reviews.apache.org/r/42126/#comment174573> IMHO this line removes the value of the interface indirection. Are there follow-ups to this patch where you intend other bodies of code will only rely on the interface? src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 128) <https://reviews.apache.org/r/42126/#comment174574> Rather than having the constructor throw, consider creating a factory function that creates an `OfferAllocation`. Additionally, please declare `throws InsufficientResourcesException` on the method signature. src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 131) <https://reviews.apache.org/r/42126/#comment174575> ditto re: final src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 201) <https://reviews.apache.org/r/42126/#comment174581> Is `clearResources()` necessary here? I think it makes sense to not intervene here if an operator with a custom config wants to specify resources, even if it's at their peril. src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java (line 49) <https://reviews.apache.org/r/42126/#comment174584> I suggest you use the non-builder types in test cases. Mutability can make these types of tests become brittle over time. With a helper function, you should be able to produce an equally-concise test case that uses all immutable `Resource` objects. ``` private static Resource makeResouce(String role) { ... } ``` src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java (line 69) <https://reviews.apache.org/r/42126/#comment174589> I generally prefer to avoid accessing private functions for the purposes of tests (unless it's prohibitive). Seems like we can infer correct sort/filter behavior from the contents of `OfferAllocation` instances. Does it make sense to only use the public API in that case? src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java (line 82) <https://reviews.apache.org/r/42126/#comment174586> `ImmutableList.of()` can stand in for `Lists.newArrayList()` here and below. src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java (line 85) <https://reviews.apache.org/r/42126/#comment174587> The index-based access is probably not going to age well, especially given that the `addAll` calls above can technically add mulitple elements. While it will be more verbose, consider using variables for each of values represented by the indexed elements. - Bill Farner On Jan. 10, 2016, 9:41 p.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42126/ > ----------------------------------------------------------- > > (Updated Jan. 10, 2016, 9:41 p.m.) > > > Review request for Aurora, Maxim Khutornenko and Bill Farner. > > > Bugs: AURORA-1109 > https://issues.apache.org/jira/browse/AURORA-1109 > > > Repository: aurora > > > Description > ------- > > This review is a prototype for introducing multiple role support in Aurora. > This creates a new interface OfferAllocation (with implementation), which > allcoates resources to resources field in TaskInfo and ExecutorInfo from an > offer. > > Current implementation prefers reserved resources over shared resources ('*' > role) if both are present. This can be customized with a differnet > comparator from PREFER_RESERVED in the future when needed. > > Several caveats: > 1. This performs the allocate after scheduling decision in > TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency > and late failure; > 2. Old code used to construct resources has not been cleaned up yet; > 3. OfferAllocationImpl's constructor allows to throw, which is a bit awkward. > > > Diffs > ----- > > src/main/java/org/apache/aurora/scheduler/OfferAllocation.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/ResourceSlot.java > 7c3d681c216b78eeecebbe950186e5a79c6fe982 > src/main/java/org/apache/aurora/scheduler/Resources.java > db422a959ee7b982c2a44323de41ad75d1a40754 > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > 8fdadda67478bb3110aa442b7d78493cf9c3edb4 > src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java > 7e8e456e288986eb0ce92a123b294e1e25d8ed18 > src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java > PRE-CREATION > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java > a5793bffabf4e5d6195b1b99f2363d241c0cecf9 > src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java > 3cbe9acd75def14ae2e0986914ba621fb164b3e4 > > Diff: https://reviews.apache.org/r/42126/diff/ > > > Testing > ------- > > Only testing with new and existing unit test. I'll work on more integration > test once we decide that this is the proper direction. > > > Thanks, > > Zhitao Li > >
