This is an automatically generated e-mail. To reply, visit:

src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 40)

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

src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 62)

    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: 
    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);

src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (lines 76 - 97)

    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)
    List<Resource.Builder> availableResources = 
    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 

src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 110)

    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?

src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 199)

    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(
          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))
    Then most call sites become `filterNonRevocableAndSort`, and the call for 
CPU passes `Resources.REVOCABLE`.  Does that make sense?

- Bill Farner

On Jan. 11, 2016, 6:29 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, 6:29 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/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 
>   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