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