----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42126/#review114287 -----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 79) <https://reviews.apache.org/r/42126/#comment175108> I'd expect this value populated from the command line arg supplied to driver setting module. Mind explaining the logic here? Why do we default to "*"? src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 108) <https://reviews.apache.org/r/42126/#comment175104> I'd put private and final fields to the top as this better follows our usual arrangement. src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 113) <https://reviews.apache.org/r/42126/#comment175103> +newline after last arg here and in other places where args spill over. src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 120) <https://reviews.apache.org/r/42126/#comment175105> Pull 'throws' to the previous line and insert newline after it src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (lines 182 - 184) <https://reviews.apache.org/r/42126/#comment175107> Does not Mesos expect resource role set when accepting an offer? Otherwise, how does it know what resource (from what role) is actually accepted? src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java (line 94) <https://reviews.apache.org/r/42126/#comment175130> Does it have to be a default here? IMO, the less intrusive approach would be allow it to be empty and not populate framework role at all. This could be similar to FRAMEWORK_AUTHENTICATION_FILE approach and would result in a zero risk upgrade cycle. - Maxim Khutornenko On Jan. 13, 2016, 10:32 p.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42126/ > ----------------------------------------------------------- > > (Updated Jan. 13, 2016, 10:32 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 > ----- > > NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b > 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 > >
