> On Jan. 11, 2016, 4:25 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line > > 201 > > <https://reviews.apache.org/r/42126/diff/1/?file=1191437#file1191437line201> > > > > 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.
This is at least necessary to get current tests pass. IIRC this is due to how Aurora handles resource limit of executor right now: 1. A prototype of `ExecutorInfo` is created in `ExecutorSettings`; 2. A `ResourceSlot` representing the usage is computed right before when we need to allocate resources from an offer; 3. The `ResourceSlot` is then passed to my new implementation to allocate resources properly next to `TaskInfo`'s. We would need to change this workflow to make sure there cannot be previously set resources if we want to avoid the `clearResources()` call. Let me know if you think tackling this workflow change should be part of this change. > On Jan. 11, 2016, 4:25 p.m., Bill Farner wrote: > > src/test/java/org/apache/aurora/scheduler/OfferAllocationImplTest.java, > > line 69 > > <https://reviews.apache.org/r/42126/diff/1/?file=1191439#file1191439line69> > > > > 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? I think sort/filter behavior can be inferred. Certain more complicated member functions might benefit from dedicated test cases so we don't mingle that with constructing a fake `Offer` and `ResourceSlot`s. I'll bear this in mind when improving this code in current iteration. - Zhitao ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42126/#review113768 ----------------------------------------------------------- On Jan. 11, 2016, 8:22 p.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42126/ > ----------------------------------------------------------- > > (Updated Jan. 11, 2016, 8:22 p.m.) > > > Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, 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 > >
