> 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
> 
>

Reply via email to