> On Jan. 13, 2015, 11:29 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 1380-1381 > > <https://reviews.apache.org/r/29869/diff/1/?file=820049#file820049line1380> > > > > Curious, why you want acceptOffers() call into this method instead of > > the current way. I imagine each operation to be implemented as a > > function/method, similar to what you did in Master::call(). > > Ben Mahler wrote: > Yep, that's what we're aiming for, we're thinking of running through the > operations and creating a chain of computation. > > Vinod Kone wrote: > maybe i'm reading the comment wrong, but isn't the end goal here not to > remove Master::launchTasks() method but to remove the installation of > LaunchTasksMessage? in my mind, the fully implemented receive() will look > like this: > > ``` > void Master::receive( > const UPID& from, > const FrameworkInfo& frameworkInfo, > const scheduler::Call::Accept& accept) { > > for (const Operation operation, accept.operations()) { > switch (operation.type()) { > case Offer::Operation::LAUNCH: > launchTasks(...); > break; > case Offer::Operation::RESERVE: > reserveResources(...); > break; > case Offer::Operation::UNRESERVE: > unreserveResources(...); > break; > case Offer::Operation::Create: > createVolume(...); > break; > case Offer::Operation::Destroy: > destroyVolume(...); > break; > } > } > } > ``` > > Ben Mahler wrote: > Not sure if we can structure it that way, given `launchTasks` is > asynchronous and we likely want to respect the ordering of operations. > > We might want to chain the operations, for example: > > ``` > Future<Nothing> chain; > > for (const Operation operation, accept.operations()) { > switch (operation.type()) { > case Offer::Operation::LAUNCH: > chain.then(defer(self(), &Master::launchTasks, ...)); // Returns a > Future once the task is launched fully. > break; > case Offer::Operation::RESERVE: > chain.then(defer(self(), &Master::reserveResources, ...)); > break; > case Offer::Operation::UNRESERVE: > chain.then(defer(self(), &Master::unreserveResources, ...)); > break; > case Offer::Operation::Create: > chain.then(defer(self(), &Master::createVolume, ...)); > break; > case Offer::Operation::Destroy: > chain.then(defer(self(), &Master::destroyVolume, ...)); > break; > } > } > ``` > > I'm not sure and it needs some exploration. We might be able to go with > something like you suggested if there's enough happening synchronously.
I've updated the TODO here to be un-opinionated about the approach we take. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29869/#review67970 ----------------------------------------------------------- On Jan. 14, 2015, 1:25 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29869/ > ----------------------------------------------------------- > > (Updated Jan. 14, 2015, 1:25 a.m.) > > > Review request for mesos, Jie Yu, Michael Park, and Vinod Kone. > > > Repository: mesos-git > > > Description > ------- > > This is an initial stub that only handles accepting offers. > > The plan (per the TODO) is to have a single path for launching tasks through > `acceptOffers`, as opposed to `launchTasks`. > > > Diffs > ----- > > src/master/master.hpp 26116aff1e965501c8d94ea0b5bd1be37f944887 > src/master/master.cpp 63ca19ab9618feccd93a2335f9287122a4665c5e > > Diff: https://reviews.apache.org/r/29869/diff/ > > > Testing > ------- > > The code path currently cannot be executed. Tests will be added once the > scheduler driver supports accepting offers. > > make check > > > Thanks, > > Ben Mahler > >
