----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14669/#review27583 -----------------------------------------------------------
src/java/src/org/apache/mesos/MesosSchedulerDriver.java <https://reviews.apache.org/r/14669/#comment53542> Reorder these alphabetically. src/java/src/org/apache/mesos/SchedulerDriver.java <https://reviews.apache.org/r/14669/#comment53543> Update the comment. src/master/master.cpp <https://reviews.apache.org/r/14669/#comment53544> Can you also include the expected slave id? src/master/master.cpp <https://reviews.apache.org/r/14669/#comment53545> include the invalid resource in the error string? src/master/master.cpp <https://reviews.apache.org/r/14669/#comment53546> Not yours, but can you include the slave id here? src/master/master.cpp <https://reviews.apache.org/r/14669/#comment53547> s/is/are/ src/master/master.cpp <https://reviews.apache.org/r/14669/#comment53548> include the expected framework id. src/master/master.cpp <https://reviews.apache.org/r/14669/#comment53549> include slave id. src/master/master.cpp <https://reviews.apache.org/r/14669/#comment53552> s/sharedSlave/slave/ You can name the 'slave' argument above as '_slave' src/master/master.cpp <https://reviews.apache.org/r/14669/#comment53555> Why not 'break' out of the for loop here? src/master/master.cpp <https://reviews.apache.org/r/14669/#comment53558> CHECK_NOTNULL(slave); src/sched/sched.cpp <https://reviews.apache.org/r/14669/#comment53562> Why do it this way instead of just doing foreach (const OfferID& offerId, offerIds) { message.add_offer_ids()->MergeFrom(offerId); } Everything else could be same as before. This avoids go through each task for every offer. src/sched/sched.cpp <https://reviews.apache.org/r/14669/#comment53560> not yours, but can you print the task id? src/sched/sched.cpp <https://reviews.apache.org/r/14669/#comment53561> not yours, but can you print the task id? src/sched/sched.cpp <https://reviews.apache.org/r/14669/#comment53563> kill this per comment above. src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment53564> thank you! src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment53565> Can you just use 'fullSlave' here? src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment53575> s/slave1/slave/ src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment53566> you can do 'resources1.cpus()' src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment53567> you can do 'resources1.mem()' src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment53569> ditto. src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment53570> ditto. src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment53571> s/combinedOffers1/combinedOffers/ src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment53572> Great test! src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment53579> this seems unused expect below. Can you kill it and explicitly define 'fullSlave'? src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment53573> ditto. use 'fullSlave' src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment53574> ditto src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment53577> ditto. src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment53576> new line. src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment53580> ditto. src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment53578> ditto. - Vinod Kone On Oct. 22, 2013, 5:37 p.m., Niklas Nielsen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14669/ > ----------------------------------------------------------- > > (Updated Oct. 22, 2013, 5:37 p.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 > >
