----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14669/#review27071 -----------------------------------------------------------
include/mesos/scheduler.hpp <https://reviews.apache.org/r/14669/#comment52696> This comment needs to be updated. Haven't looked at the rest of the review, but what are the semantics here? Is each offer in offerIds supposed to be mapped to the corresponding index in the tasks list? src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp <https://reviews.apache.org/r/14669/#comment52697> If you change the signature we would have backwards compatibility issues. For example if framework writers use old jar with old launchTasks() and mesos libs are upgraded to use the new launchTasks(). We should have the deprecated jni method for old launchTasks() for atleast one version before we can kill it. Makes sense? src/java/src/org/apache/mesos/MesosSchedulerDriver.java <https://reviews.apache.org/r/14669/#comment52698> You probably want to reorder these two. src/master/master.hpp <https://reviews.apache.org/r/14669/#comment52700> Can you update the comment? src/master/master.cpp <https://reviews.apache.org/r/14669/#comment52709> s/getCombinedResources/resources/ ? src/master/master.cpp <https://reviews.apache.org/r/14669/#comment52702> s/Attempt to get resources from empty/Empty/ ? src/master/master.cpp <https://reviews.apache.org/r/14669/#comment52703> s/list/list./ src/master/master.cpp <https://reviews.apache.org/r/14669/#comment52704> Pull this down after the framework and slave checks. That way you won't be unnecessarily putting something in the map if you are going to error out immediately. src/master/master.cpp <https://reviews.apache.org/r/14669/#comment52706> Is this even possible? A framework cannot possibly know offer ids of offers that were not sent to it? src/master/master.cpp <https://reviews.apache.org/r/14669/#comment52708> Why pull this out into a temporary? src/master/master.cpp <https://reviews.apache.org/r/14669/#comment52714> How about a generic (templated) output operator for vector<T> ? src/master/master.cpp <https://reviews.apache.org/r/14669/#comment52711> new line src/master/master.cpp <https://reviews.apache.org/r/14669/#comment52758> This function needs some refactoring now that it takes vector<Offer>. This is what I propose we do: // Merge processTasks() in to launchTasks(). launchTasks() { // Validate offers. Perhaps we can write OfferValidators like we do for task validators. // Validate tasks. // launchTask(). } I suspect this will also eliminate/simplify some of the helpers that were added (e.g., reportInvalidOffers(), combinedResources()) Does that make sense? src/master/master.cpp <https://reviews.apache.org/r/14669/#comment52715> Why not take Framework* as parameter? src/master/master.cpp <https://reviews.apache.org/r/14669/#comment52716> s/combinedResourcesTry/totalResources/ src/master/master.cpp <https://reviews.apache.org/r/14669/#comment52717> It seems odd to do this check this late. How about ensuring we have valid offers before processTasks() is called instead? src/python/native/mesos_scheduler_driver_impl.cpp <https://reviews.apache.org/r/14669/#comment52718> s/offerArgument/offerIdsObj/ src/python/native/mesos_scheduler_driver_impl.cpp <https://reviews.apache.org/r/14669/#comment52719> How is this possible? src/sched/sched.cpp <https://reviews.apache.org/r/14669/#comment52725> Not yours, but can you log the slave id? src/sched/sched.cpp <https://reviews.apache.org/r/14669/#comment52726> Not yours, but can you log the offer id? src/sched/sched.cpp <https://reviews.apache.org/r/14669/#comment52720> Why did you pull this out? I would put it on line #787. src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment52733> consider Resources fullSlave = halfSlave + halfSlave; Resources twoSlaves = fullSlave + fullSlave; src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment52734> s/notified with/notified/ src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment52735> s/offer/offer./ src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment52737> You could also check the resources match what you expect. src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment52738> You could also check resources match what you expect. src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment52739> Split this into its own test. This test is huge! src/tests/master_tests.cpp <https://reviews.apache.org/r/14669/#comment52740> Ditto. Split this into its own test. - Vinod Kone On Oct. 16, 2013, 6:15 a.m., Niklas Nielsen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14669/ > ----------------------------------------------------------- > > (Updated Oct. 16, 2013, 6:15 a.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone. > > > 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 1bf5d47 > src/messages/messages.proto a5dded2 > src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d > src/sched/sched.cpp 824b4b7 > src/tests/master_tests.cpp feea541 > > Diff: https://reviews.apache.org/r/14669/diff/ > > > Testing > ------- > > A new test, MasterTest.LaunchCombinedOfferTest, has been added. > This test ensures that: > 1) Multiple offers can be used to run a single task (requesting the sum of > offer resources). > 2) No offers can appear more than once in offer list. > 3) Offers cannot span multiple slaves. > > $ make check > ... > [ RUN ] MasterTest.LaunchCombinedOfferTest > [ OK ] MasterTest.LaunchCombinedOfferTest (3043 ms) > ... > > > Thanks, > > Niklas Nielsen > >
