> On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote: > > include/mesos/scheduler.hpp, lines 238-246 > > <https://reviews.apache.org/r/14669/diff/2/?file=365052#file365052line238> > > > > 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? > > > > > >
Offers are aggregated into one resource and treated similar to resources from a single offer. > On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote: > > src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp, lines 710-711 > > <https://reviews.apache.org/r/14669/diff/2/?file=365053#file365053line710> > > > > 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? Definitely! I was struggling with method overloading in JNI, but I'll take a second look at it. The two method signatures (method names are mangled in case of overloading) was generated just fine, but caused LinkError in the JVM anyway. > On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote: > > src/master/master.cpp, line 726 > > <https://reviews.apache.org/r/14669/diff/2/?file=365057#file365057line726> > > > > Why pull this out into a temporary? No need. It has been killed :) > On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 1101-1108 > > <https://reviews.apache.org/r/14669/diff/2/?file=365057#file365057line1101> > > > > How about a generic (templated) output operator for vector<T> ? stringify(vector<>) already exists, so I killed this helper. > On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote: > > src/master/master.cpp, line 2178 > > <https://reviews.apache.org/r/14669/diff/2/?file=365057#file365057line2178> > > > > It seems odd to do this check this late. How about ensuring we have > > valid offers before processTasks() is called instead? If offerId appears multiple times in offerIds, without an explicit check, a offer can be tried to removed multiple times which causes a fault. > On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote: > > src/sched/sched.cpp, lines 808-810 > > <https://reviews.apache.org/r/14669/diff/2/?file=365060#file365060line808> > > > > Why did you pull this out? I would put it on line #787. tasks will be added |offerIds| times if we leave it in the loop above. > On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote: > > src/master/master.hpp, lines 171-173 > > <https://reviews.apache.org/r/14669/diff/2/?file=365056#file365056line171> > > > > Can you update the comment? This method has been merged into launchTasks() and I killed the comment. > On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 1110-1149 > > <https://reviews.apache.org/r/14669/diff/2/?file=365057#file365057line1110> > > > > 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? Definitely! I gave it a shot and merged in processTasks() and introduced OfferVisitor scheme. > On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote: > > src/python/native/mesos_scheduler_driver_impl.cpp, lines 414-416 > > <https://reviews.apache.org/r/14669/diff/2/?file=365059#file365059line414> > > > > How is this possible? This was copied from the task list generation just below. You are right - we should be able to trust the index (as that is the only case we get a NULL pointer from PyList_GetItem. Would you suggest we remove both tests? > On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote: > > src/master/master.cpp, line 677 > > <https://reviews.apache.org/r/14669/diff/2/?file=365057#file365057line677> > > > > s/getCombinedResources/resources/ ? resources()/getCombinedResources() has been killed. - Niklas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14669/#review27071 ----------------------------------------------------------- On Oct. 17, 2013, 6:10 p.m., Niklas Nielsen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14669/ > ----------------------------------------------------------- > > (Updated Oct. 17, 2013, 6:10 p.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 > >
