----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56690/#review166752 -----------------------------------------------------------
How does this work with preemption? Should we reject a configuration where tasks can be preempted and reserve resources? src/main/java/org/apache/aurora/scheduler/TierInfo.java (line 73) <https://reviews.apache.org/r/56690/#comment238788> I noticed from the code below that the mutator methods only act on copies of the teir info. Have you considered having the mutator methods only return copies? This means that the impementations of `unReserve` `reReserve` only return copies. This would be good for performance because we no longer need to return a copy of tier info for every access, and we only do the copying when necessary. src/main/java/org/apache/aurora/scheduler/TierInfo.java (line 76) <https://reviews.apache.org/r/56690/#comment238789> Could we condense `unReserve()` and `reReserve` to just be `setReservation(bool b)`? I think it is more inline with our style elsewhere for mutators.getTier src/main/java/org/apache/aurora/scheduler/TierInfo.java (line 108) <https://reviews.apache.org/r/56690/#comment238790> If you accept my suggestion above about making copying implicit in the mutators, this would need to become private. src/main/java/org/apache/aurora/scheduler/TierManager.java (line 105) <https://reviews.apache.org/r/56690/#comment238791> In line with my comments above, it is important to note this returns a copy for every access which is sort of undesriable considering how frequently we do this in the scheduling loop. src/main/java/org/apache/aurora/scheduler/base/JobKeys.java (line 114) <https://reviews.apache.org/r/56690/#comment238792> Whats the reason for the `components.size() == 4` here? src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java (line 201) <https://reviews.apache.org/r/56690/#comment238795> Could you elaborate on what you mean by "cluster maintenance" here? Seems like a copy and paste error from above. src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java (line 270) <https://reviews.apache.org/r/56690/#comment238798> `Optional<List<T>>` is a smell to me. Is it possible to model this just as `List<T>` where the empty list is equal to `Optional.absent()`? src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java (line 315) <https://reviews.apache.org/r/56690/#comment238799> This needs javadoc. src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java (line 332) <https://reviews.apache.org/r/56690/#comment238800> This equals is not the same as the other POJOs. Look at `UnusedResource` for inspiration. src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java (line 85) <https://reviews.apache.org/r/56690/#comment238801> If you take my suggestion above to model `resourceList` as `List<T>` I think this code would be cleaned up and be easier to read. src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 122) <https://reviews.apache.org/r/56690/#comment238803> You need to do `this.driverSettings = requireNonNull(driverSettings)` here to guard against null pointers. src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 358) <https://reviews.apache.org/r/56690/#comment238804> A comment here explaining that we want to label all resources would make this bit clearer. I don't think we do `clearXXX().addAllXXX()` anywhere else in the code. Also does protobuf not provide a `setXXX` for collections? src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java (line 365) <https://reviews.apache.org/r/56690/#comment238805> You need to log the task id for this line to provide enough information for someone debugging a live cluster. src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java (line 135) <https://reviews.apache.org/r/56690/#comment238806> The comment here and the type don't match. src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java (line 186) <https://reviews.apache.org/r/56690/#comment238807> This seems like a formatting error. Please revert it. src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java (line 405) <https://reviews.apache.org/r/56690/#comment238808> I think we can remove this line here. src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java (line 416) <https://reviews.apache.org/r/56690/#comment238809> Is there a reason why we need the reserve op to be at index 0? Could we instead just do `.add()`? `List<T>#add(int i, T)` is a smell to me. src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java (line 418) <https://reviews.apache.org/r/56690/#comment238810> I think we can make this .info and list the task id, offer and operations. src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java (line 34) <https://reviews.apache.org/r/56690/#comment238811> Indentation is off here, I think we want one argument per line (as it was before). src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java (line 47) <https://reviews.apache.org/r/56690/#comment238813> I think replacing "reserved task" with "task with resoruced resources" is clearer. src/main/java/org/apache/aurora/scheduler/resources/ResourceManager.java (line 141) <https://reviews.apache.org/r/56690/#comment238812> Lets remove the commented out code here? src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java (line 155) <https://reviews.apache.org/r/56690/#comment238814> It sems we are only doing read only operations here, so lets change the sigature to only accept the `StoreProvider` interface for safety. Also, maybe we should rename this to "hasReachedReservationTimeout"? It's not clear what we are "waiting" for in the name. src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java (line 292) <https://reviews.apache.org/r/56690/#comment238815> I think a comment here explaining that we do not want to veto offers for reservation reasons because after we reach the timeout we can reserve it. src/test/java/org/apache/aurora/scheduler/TierManagerTest.java (line 27) <https://reviews.apache.org/r/56690/#comment238816> Can you revert the import glob here? - Zameer Manji On Feb. 23, 2017, 4:28 p.m., Dmitriy Shirchenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56690/ > ----------------------------------------------------------- > > (Updated Feb. 23, 2017, 4:28 p.m.) > > > Review request for Aurora, Mehrdad Nurolahzade, Stephan Erb, and Zameer Manji. > > > Bugs: AURORA-1819 > https://issues.apache.org/jira/browse/AURORA-1819 > > > Repository: aurora > > > Description > ------- > > This is an RFC (without tests) for dynamic reservations proposal. If there is > consensus on the approach, I will add tests. This patch was also tested > locally and works as expected. > > > Diffs > ----- > > src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java > f2296a9d7a88be7e43124370edecfe64415df00f > src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java > 6f2ca35c5d83dde29c24865b4826d4932e96da80 > src/main/java/org/apache/aurora/scheduler/TaskVars.java > 676dfd9f9d7ee0633c05424f788fd0ab116976bb > src/main/java/org/apache/aurora/scheduler/TierInfo.java > c45b949ae7946fc92d7e62f94696ddc4f0790cfa > src/main/java/org/apache/aurora/scheduler/TierManager.java > c6ad2b1c48673ca2c14ddd308684d81ce536beca > src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java > b12ac83168401c15fb1d30179ea8e4816f09cd3d > src/main/java/org/apache/aurora/scheduler/base/JobKeys.java > 0136afb8f6049a6d88cd42b5e3f17d61fcd629d5 > src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java > f0b148cd158d61cd89cc51dca9f3fa4c6feb1b49 > > src/main/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilter.java > f6c759f03c4152ae93317692fc9db202fe251122 > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java > bb1a960a4c77f48b0ceaa213bd27546551f384f9 > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java > 60097d91d836e2686d6e90571f13a2fbfd88ae14 > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > 0d639f66db456858278b0485c91c40975c3b45ac > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java > 8c000cb0626bd34f6f30e23fe2b3a045f2b44e35 > src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java > e16e36ed360ef9ca371df9084365ea88cfb6e7ce > src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java > 202cae96ffc5b49e638b973a273f7983137b5baf > src/main/java/org/apache/aurora/scheduler/resources/ResourceManager.java > 9aa263a9cfae03a9a0c5bc7fe3a1405397d3009c > src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java > 203f62bacc47470545d095e4d25f7e0f25990ed9 > src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java > da378e84ee65a658ff2382489d3ab6d5f6451b5f > src/main/resources/org/apache/aurora/scheduler/tiers.json > 34ddb1dc769a73115c209c9b2ee158cd364392d8 > src/test/java/org/apache/aurora/scheduler/TierManagerTest.java > 82e40d509d84c37a19b6a9ef942283d908833840 > src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java > 49d4e82cc03144b80292fe43066a6cc4d7aed88f > src/test/java/org/apache/aurora/scheduler/resources/AcceptedOfferTest.java > dded9c34749cf599d197ed312ffb6bf63b6033f1 > > src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java > b8b8edb1a21ba89b8b60f8f8451c8c776fc23ae8 > src/test/java/org/apache/aurora/scheduler/resources/ResourceTestUtil.java > e04f6113c43eca4555ee0719f8208d7c4ebb8d61 > src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java > cf2d25ec2e407df7159e0021ddb44adf937e1777 > > Diff: https://reviews.apache.org/r/56690/diff/ > > > Testing > ------- > > > Thanks, > > Dmitriy Shirchenko > >
