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


Thanks for the changes so far, nearly there!


src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 39)
<https://reviews.apache.org/r/42126/#comment174944>

    I did a quick rearrangement of this method (after applying the comment 
below), i think the result is a bit easier to read, and uses fewer temp 
variables.
    
    I also cheated and omitted the `requireNonNull` calls as nulls will 
immediately trigger NPE for all parameters.
    
    ```
      public static AcceptedOffer create(
          Offer offer,
          ResourceSlot taskSlot,
          ResourceSlot executorSlot,
          Set<Integer> selectedPorts,
          TierInfo tierInfo) throws Resources.InsufficientResourcesException {
    
        List<Resource> reservedFirst = ImmutableList.<Resource>builder()
            .addAll(Iterables.filter(offer.getResourcesList(), RESERVED))
            .addAll(Iterables.filter(offer.getResourcesList(), NOT_RESERVED))
            .build();
    
        boolean revocable = tierInfo.isRevocable();
        List<Resource.Builder> cpuResources = filterToBuilders(
            reservedFirst,
            ResourceType.CPUS.getName(),
            revocable ? Resources.REVOCABLE : Resources.NON_REVOCABLE);
        List<Resource.Builder> memResources = filterToBuilderNonRevocable(
            reservedFirst, ResourceType.RAM_MB.getName());
        List<Resource.Builder> diskResources = filterToBuilderNonRevocable(
            reservedFirst, ResourceType.DISK_MB.getName());
        List<Resource.Builder> portsResources = filterToBuilderNonRevocable(
            reservedFirst, ResourceType.PORTS.getName());
    
        List<Resource> taskResources = ImmutableList.<Resource>builder()
            .addAll(allocateScalarType(cpuResources, taskSlot.getNumCpus(), 
revocable))
            .addAll(allocateScalarType(memResources, 
taskSlot.getRam().as(Data.MB), false))
            .addAll(allocateScalarType(diskResources, 
taskSlot.getDisk().as(Data.MB), false))
            .addAll(allocateRangeType(portsResources, selectedPorts))
            .build();
    
        List<Resource> executorResources = ImmutableList.<Resource>builder()
            .addAll(allocateScalarType(cpuResources, executorSlot.getNumCpus(), 
revocable))
            .addAll(allocateScalarType(memResources, 
executorSlot.getRam().as(Data.MB), false))
            .addAll(allocateScalarType(diskResources, 
executorSlot.getDisk().as(Data.MB), false))
            .build();
    
        return new AcceptedOffer(taskResources, executorResources);
      }
    ```



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 119)
<https://reviews.apache.org/r/42126/#comment174949>

    feel free to declare as `List` instead of `ImmutableList`, ditto in the 
constructor.



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 129)
<https://reviews.apache.org/r/42126/#comment174950>

    This is a good general-purpose implementation of this behavior, but in 
practice it seems like overkill.  Can you instead match the behavior currently 
on master and synthesize the `Resource` with `makeMesosRangeResource(PORTS, 
selectedPorts)`?



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 177)
<https://reviews.apache.org/r/42126/#comment174948>

    Remove `final` here and throughout the method.  We only use `final` when 
necessary, or to ensure immutability of class field references.



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 178)
<https://reviews.apache.org/r/42126/#comment174938>

    This parameter is now effectively a return value, so instead make it a 
formal return type.



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 181)
<https://reviews.apache.org/r/42126/#comment174953>

    `remaining` instead of `outstanding`



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 199)
<https://reviews.apache.org/r/42126/#comment174954>

    Move this up to the line after the declaration of `used` to make for easier 
readability of the accounting logic.



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 200)
<https://reviews.apache.org/r/42126/#comment174955>

    `r.getScalarBuilder().setValue(available - used);`



src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java (line 154)
<https://reviews.apache.org/r/42126/#comment174957>

    `runMultipleRolesTest` to make it clear this is not intended to be a test 
case



src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java (line 253)
<https://reviews.apache.org/r/42126/#comment174958>

    You could simplify this a bit for callers by changing `Set<Integer>... 
values` to `Integer... values`.  This way callers don't need the 
`ImmutableSet.of()` boilerplate.  It also resolves the unchecked generic vararg 
compiler warning you have now.
    
    On this end, you will remove the `for` loop and do:
    
    ```
    resources.add(AcceptedOffer.makeMesosRangeResource(prototype.build(), 
ImmutableSet.copyOf(values));
    ```


- Bill Farner


On Jan. 12, 2016, 6:08 p.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2016, 6:08 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 class OfferAllocation, 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
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> 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/AcceptedOfferTest.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