> On Oct. 21, 2013, 6:34 p.m., Vinod Kone wrote: > > Also, please attach the JIRA issue to this review.
Cool - done. Thanks for the reviews and patience with this patch btw! > On Oct. 21, 2013, 6:34 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 1230-1231 > > <https://reviews.apache.org/r/14669/diff/7/?file=368247#file368247line1230> > > > > Not yours, but can you kill this TODO? Done. > On Oct. 21, 2013, 6:34 p.m., Vinod Kone wrote: > > src/master/master.cpp, line 1286 > > <https://reviews.apache.org/r/14669/diff/7/?file=368247#file368247line1286> > > > > Why do you need a hashmap? Is a hashset of OfferID not sufficient? No need - has been killed. > On Oct. 21, 2013, 6:34 p.m., Vinod Kone wrote: > > src/master/master.cpp, line 1315 > > <https://reviews.apache.org/r/14669/diff/7/?file=368247#file368247line1315> > > > > I think this is abusing the visitor pattern :) > > > > Lets just do offer aggregation inline in launchTasks(), or if you > > prefer do it in a helper. IIUC, this helper could be used in the future > > when we implement 'update'. I have flattened it into launchTasks for now. > On Oct. 21, 2013, 6:34 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 1392-1404 > > <https://reviews.apache.org/r/14669/diff/7/?file=368247#file368247line1392> > > > > Seems odd to do these checks for only the first offer. Can we use the > > offer validators do these checks for each offer? I have added visitors for these checks now. > On Oct. 21, 2013, 6:34 p.m., Vinod Kone wrote: > > src/master/master.cpp, line 1487 > > <https://reviews.apache.org/r/14669/diff/7/?file=368247#file368247line1487> > > > > This is not really invalid offer right? This is an invalid task. I have killed reportInvalidOffers and pulled status update sending into launchTasks. > On Oct. 21, 2013, 6:34 p.m., Vinod Kone wrote: > > src/master/master.cpp, line 1547 > > <https://reviews.apache.org/r/14669/diff/7/?file=368247#file368247line1547> > > > > Is reportInvalidOffers() called from anywhere else? If not, we don't > > need to pull it out into a helper function? As mentioned above, I killed reportInvalidOffers and pulled status update sending in. > On Oct. 21, 2013, 6:34 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 1439-1440 > > <https://reviews.apache.org/r/14669/diff/7/?file=368247#file368247line1439> > > > > This is odd. Why do it here instead of doing it in the if statement > > above? I am not sure that I follow: Is it the NULL-check of the offer or breaking on offer validation error? If we don't break, a subsequent (valid) offer check will overwrite previous bad ones. - Niklas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14669/#review27254 ----------------------------------------------------------- On Oct. 22, 2013, 12:02 a.m., Niklas Nielsen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14669/ > ----------------------------------------------------------- > > (Updated Oct. 22, 2013, 12:02 a.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone. > > > Bugs: MESOS-749 > https://issues.apache.org/jira/browse/MESOS-749 > > > Repository: mesos-git > > > Description > ------- > > Running tasks on more than one offer belonging to a single slave can be > useful in situations with multiple out-standing offers. > > This patch extends the usual launchTasks() to accept a vector of OfferIDs. > The previous launchTasks (accepting a single OfferID) has been kept for > backward compatibility, but this now calls the new launchTasks() with a > one-element list. > This also applied for the JNI and python interfaces, which accepts both > formats as well. > > Offers are verified to belong to the same slave and framework, before > resources are merged and used. > > > Diffs > ----- > > include/mesos/scheduler.hpp fa1ffe8 > src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp 9869929 > src/java/src/org/apache/mesos/MesosSchedulerDriver.java ed4b4a3 > src/java/src/org/apache/mesos/SchedulerDriver.java 93aaa54 > src/master/master.hpp 9f5e25b > src/master/master.cpp acbb137 > src/messages/messages.proto a5dded2 > src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d > src/sched/sched.cpp 824b4b7 > src/tests/master_tests.cpp bf790d2 > > Diff: https://reviews.apache.org/r/14669/diff/ > > > Testing > ------- > > Three new tests has been added: LaunchCombinedOfferTest, > LaunchAcrossSlavesTest and LaunchDuplicateOfferTest > This test ensures that: > 1) Multiple offers can be used to run a single task (requesting the sum of > offer resources). > 2) Offers cannot span multiple slaves. > 3) No offers can appear more than once in offer list. > > $ make check > ... > [ RUN ] MasterTest.LaunchCombinedOfferTest > [ OK ] MasterTest.LaunchCombinedOfferTest (2010 ms) > [ RUN ] MasterTest.LaunchAcrossSlavesTest > [ OK ] MasterTest.LaunchAcrossSlavesTest (3 ms) > [ RUN ] MasterTest.LaunchDuplicateOfferTest > [ OK ] MasterTest.LaunchDuplicateOfferTest (3 ms) > ... > > > Thanks, > > Niklas Nielsen > >
