----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29976/#review69066 -----------------------------------------------------------
Ship it! Overall looks good. I'd love to see more consistent us building on `drop` per my comments below, but please leave TODOs so we can do it in follow up reviews. src/master/master.hpp <https://reviews.apache.org/r/29976/#comment113637> 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/ src/master/master.cpp <https://reviews.apache.org/r/29976/#comment113647> 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. src/master/master.cpp <https://reviews.apache.org/r/29976/#comment113651> 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. src/master/master.cpp <https://reviews.apache.org/r/29976/#comment113654> 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 :) - Ben Mahler 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 > >
