> On Jan. 21, 2015, 11:31 p.m., Ben Mahler wrote: > > src/master/master.hpp, line 429 > > <https://reviews.apache.org/r/29976/diff/3/?file=828919#file828919line429> > > > > Since this is in other places in the master code, might be nice to > > follow up on a sweep of the following: > > > > s/offeredResources/offered/ > > s/usedResources/used/
Sure, will follow up with a patch. > On Jan. 21, 2015, 11:31 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 2766 > > <https://reviews.apache.org/r/29976/diff/3/?file=828920#file828920line2766> > > > > Not yours, mind doing s/offerValidators/validators/ ? > > > > Seems consistent with the singular 'validator' below as well. > > > > Feel free to do this as a follow up. Done. > On Jan. 21, 2015, 11:31 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 2804-2805 > > <https://reviews.apache.org/r/29976/diff/3/?file=828920#file828920line2804> > > > > Could you leverage `drop` here per my other comment? That way, we'll > > log consistently when we drop things, regardless of when we drop them. > > We'll also get consistent metrics about dropped messages! Thoughts? > > > > You can add a `drop` overload for Accept: > > ``` > > drop(accept, frameworkInfo, "Framework cannot be found"); > > ``` > > > > Consider a TODO on this version of `drop` for dealing with recovering > > resources from the offers, notice how this is becoming similar to > > scheduler.cpp! :) > > > > Would really like to have the framework info here for consistent > > logging. Added a TODO. THanks for the comment. I like it. > On Jan. 21, 2015, 11:31 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 2894-2896 > > <https://reviews.apache.org/r/29976/diff/3/?file=828920#file828920line2894> > > > > Ditto for `drop` here. > > > > ``` > > drop(accept, frameworkInfo, "Framework cannot be found"); > > ``` > > > > It's not clear if recovering the resources just below here will ever be > > necessary, since we don't know the framework! It seems nice for > > `drop(Accept, ...)` to consistently handle resource recovery, if applicable. > > > > We could even consider `drop(Accept, ...)` also dealing with lost task > > notifications, through additional overloads (possibly per operation to > > cleanup the cruft in the Offer::Operation::LAUNCH case block below), but > > one step at a time, a TODO for this latter part would be great :) Left a TODO. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29976/#review69066 ----------------------------------------------------------- On Jan. 21, 2015, 10:29 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29976/ > ----------------------------------------------------------- > > (Updated Jan. 21, 2015, 10:29 p.m.) > > > Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone. > > > Repository: mesos-git > > > Description > ------- > > Moved launchTasks logics to ACCEPT call handler in master. > > This patch moves all logics of launchTasks in master to the ACCEPT call > handler in master, and leaves stubs for other offer operations. We still > keeps the launchTasks handler for backwards compatibility. launchTasks will > simply create an Accept call message, and invoke the ACCEPT call handler. > > > Diffs > ----- > > src/master/master.hpp a8ce4d023ddea36cb83a2dc7b95abd12342f345a > src/master/master.cpp e9dcca3c92c94f3123519855e238bcef47eeece9 > src/tests/resource_offers_tests.cpp > d098e7016ac0da7f1d629af099bb1b8fa66da839 > > Diff: https://reviews.apache.org/r/29976/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
