> On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote: > > Cool! Sorry for taking so long to look at this! > > > > We should dogfood our own API changes, can you send out a subsequent change > > for all of the test frameworks to update any calls to launchTasks?
Thanks for the review Ben! You bet, I'll follow up with a new review request. > On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote: > > include/mesos/scheduler.hpp, lines 242-249 > > <https://reviews.apache.org/r/14669/diff/13/?file=373620#file373620line242> > > > > Just an observation, this may prove to be slightly confusing for the > > users of this API. > > > > If they don't hold on to offers, they can't possibly place two offers > > into a vector and make a valid call to launchTasks. Perhaps we should > > clarify that the only way for a framework to have multiple offers for the > > same slave is if they're holding on to offers. > > > > Seems good for now, in the future, having a document in the /docs/ > > folder that explains the offer model in Mesos would be great for framework > > developers. Noted. > On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote: > > src/java/src/org/apache/mesos/SchedulerDriver.java, lines 110-122 > > <https://reviews.apache.org/r/14669/diff/13/?file=373623#file373623line110> > > > > Curious as to why you used @deprecated earlier but you're not using > > @deprecated on these two? No particular reason. The new patch use @deprecated for those too. > On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 1021 > > <https://reviews.apache.org/r/14669/diff/13/?file=373625#file373625line1021> > > > > Have you seen post-reviews.py? It will allow you to break apart your > > diffs into multiple patches (for example it looks like you moved the > > visitors above launchTasks, so in this diff it's difficult to see what's > > changed in the visitors). > > > > As an example, you could break these into two commits and send two > > separate reviews very easily with post-reviews.py. :) Sorry about that. In this case, processTasks() was merged into launchTasks() so the task visitors either needed to be prototyped or moved above launchTasks. > On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1250-1253 > > <https://reviews.apache.org/r/14669/diff/13/?file=373625#file373625line1250> > > > > Does the typedef prevent you from directly returning the string, using > > the implicit Option<T> constructor? > > > > return "Offer " + ...; > > > > Ditto for SlaveChecker and UniqueOfferIDChecker. Ah, right - good point! I was too heavily influenced by the old visitors :) > On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 1256 > > <https://reviews.apache.org/r/14669/diff/13/?file=373625#file373625line1256> > > > > Does this work with the typedef? > > > > return None(); > > > > Ditto for SlaveChecker and UniqueOfferIDChecker. Thanks! done > On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 1263 > > <https://reviews.apache.org/r/14669/diff/13/?file=373625#file373625line1263> > > > > This sounds like UniqueSlaveChecker, although I'm surprised to see this > > checking for null pointers :( > > > > Can this take const& for all of these? > > > > Looking at TaskInfoChecker, even though it takes pointers, none of the > > visitors are checking for NULL and it seems they should be taking const > > references instead. Feel free to make that change here or leave as is, but > > for the OfferVisitors can you take const& for the arguments? I changed the arguments to be const references instead and removed the null check. > On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1323-1328 > > <https://reviews.apache.org/r/14669/diff/13/?file=373625#file373625line1323> > > > > What if we broke up the validation steps? > > > > First the offers are validated, then cleanup the offer visitors after > > this point (rather than at the very end with the task visitors). If there > > was an error, we have to remove the offers and return. > > > > Now, we validate the tasks, launching the valid ones. Then, always > > cleanup the task visitors / allocator / offers. > > > > Let's try to flatten the control flow a bit and avoid the nested break > > statements. Good point, I moved the offer validation tear-down up and flattened the control flow. I haven't been able to get rid of the nested break unfortunately. > On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 1335 > > <https://reviews.apache.org/r/14669/diff/13/?file=373625#file373625line1335> > > > > Definitely! Maybe a better place for this kind of TODO is on install() > > so that others find it more easily? I will follow up with a subsequent patch with this todo. > On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 1380 > > <https://reviews.apache.org/r/14669/diff/13/?file=373625#file373625line1380> > > > > Let's handle the NULL slave here instead? Would be great if the Visitor > > can take const references instead of pointers. Yep - done. > On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 1521 > > <https://reviews.apache.org/r/14669/diff/13/?file=373625#file373625line1521> > > > > Can you add a CHECK_NOTNULL in removeOffer()? CHECK_NOTNULL will fail if you give a list of offers with duplicates. I thought it would be more graceful to ignore it. Do you still think we should assert? - Niklas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14669/#review28129 ----------------------------------------------------------- On Nov. 13, 2013, 10:19 p.m., Niklas Nielsen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14669/ > ----------------------------------------------------------- > > (Updated Nov. 13, 2013, 10:19 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 380e087 > 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 5b0ca39 > src/master/master.hpp e377af8 > src/master/master.cpp 8e14a07 > src/messages/messages.proto a5dded2 > src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d > src/sched/sched.cpp 3abe72f > src/tests/master_tests.cpp bf790d2 > src/tests/resource_offers_tests.cpp 2864c9a > > 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 > >
