> On Jan. 12, 2016, 6:05 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 40
> > <https://reviews.apache.org/r/42126/diff/2/?file=1192873#file1192873line40>
> >
> >     The interface should really be removed.  It's not acting as an 
> > interface hiding an implementation, so its value is not worth its weight.  
> > Everything else can remain as-is (e.g. static factory function).

Will do.


> On Jan. 12, 2016, 6:05 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 62
> > <https://reviews.apache.org/r/42126/diff/2/?file=1192873#file1192873line62>
> >
> >     Please move ~all of this method body into `create` to avoid doing 
> > significant work in the constructor.
> >     
> >     Background on this convention can be found here if you're interested: 
> > http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/
> >     
> >     The constructor should whittle down to something as basic as:
> >     
> >     ```
> >     AcceptedOffer(ImmutableList<Resource> taskResources, 
> > ImmutableList<Resource> executorResources) {
> >       this.taskResources = requireNonNull(taskResources);
> >       this.executorResources = requireNonNull(executorResources);
> >     }
> >     ```

Will do.


> On Jan. 12, 2016, 6:05 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 199
> > <https://reviews.apache.org/r/42126/diff/2/?file=1192873#file1192873line199>
> >
> >     I now see why you do indeed need _some_ flag for `revocable` because 
> > both `REVOCABLE` and `NON_REVOCABLE` filters are active (neither are a 
> > no-op).  I suggest that you accept `Predicate<Resource>` and introduce an 
> > overload to make the call sites more readable (`false` does not indicate 
> > much)
> >     
> >     ```
> >     static List<Resource> filterNonRevocableAndSort(List<Resource> 
> > resources, String name) {
> >       return filterAndSort(
> >           resources,
> >           name,
> >           revocable ? Resources.REVOCABLE : Resources.NON_REVOCABLE);
> >     }
> >     
> >     static List<Resource> filterAndSort(
> >         List<Resource> resources,
> >         String name,
> >         Predicate<Resource> additionalFilter) {
> >     
> >       return FluentIterable.from(resources)
> >           .filter(e -> e.getName().equals(name))
> >           .filter(additionalFilter)
> >           .toSortedList(PREFER_RESERVED);
> >     }
> >     ```
> >     
> >     Then most call sites become `filterNonRevocableAndSort`, and the call 
> > for CPU passes `Resources.REVOCABLE`.  Does that make sense?

Yes this makes sense. The only difference I'd like is to make this return an 
`ArrayList` instead of `ImmutableList` from `toSortedList` because this list 
needs to be mutable so we can allocate resources from it.


> On Jan. 12, 2016, 6:05 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, lines 76-97
> > <https://reviews.apache.org/r/42126/diff/2/?file=1192873#file1192873line76>
> >
> >     A few thoughts for this code block:
> >     1. rather than sort, filter reserved resources and arrange them first.  
> > the sort is extra work, and currently defines an arbitrary sub-sort on role 
> > name which is misleading at best.
> >     2. convert to List<Resource.Builder> upfront and use these to track the 
> > remaining capacity of the resource by mutating them
> >     
> >     If you agree, you would start this block with something like
> >     ```
> >     List<Resource> reservedFirst = ImmutableList.<Resource>builder()
> >       .addAll(Iterables.filter(offer.getResourcesList(), RESERVED)
> >       .addAll(Iterables.filter(offer.getResourcesList(), NOT_RESERVED)
> >       .build()
> >     
> >     List<Resource.Builder> availableResources = 
> > FluentIterable.from(reservedFirst)
> >         .transform(Resource::toBuilder)
> >         .toList();
> >     ```
> >     
> >     This will also allow you to avoid modifying the `List` itself (as is 
> > currently done below with `ListIterator.remove/add`) since you can adjust 
> > the remaining capacity in `Resource.Builder` instances rather than 
> > constructing new ones.

Will do.


> On Jan. 12, 2016, 6:05 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 110
> > <https://reviews.apache.org/r/42126/diff/2/?file=1192873#file1192873line110>
> >
> >     I'd like to strive to remove `@VisibleForTesting` from all methods in 
> > this class and make them `private`.  It will likely require more 
> > fixtures/helpers in the unit test, but the result will be a unit test that 
> > is not so highly coupled to the implementation.  Do you see anything 
> > preventing this approach?

Will do. I think I just need one helper function to be package private so I 
don't repeat it in test.


- Zhitao


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42126/#review114000
-----------------------------------------------------------


On Jan. 12, 2016, 2:29 a.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2016, 2:29 a.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/AcceptedOffer.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/CommandLineDriverSettingsModule.java
>  2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
>   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/AcceptedOfferImplTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> 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
> -------
> 
> 1. Unit tested with old and new tests;
> 2. vagrant integration tests: I manually separate out the vagrant box's cpu 
> and memory between 'aurora-test' role and '*' and verified that jobs can 
> still be launched (I can post the vagrant change in another follow upon 
> request).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Reply via email to