> On Nov. 30, 2017, 2:55 a.m., Jie Yu wrote: > > src/master/master.cpp > > Lines 7426 (patched) > > <https://reviews.apache.org/r/63732/diff/9/?file=1903718#file1903718line7426> > > > > See my comment in https://reviews.apache.org/r/63842/ > > > > since removeOfferOperation will update 'used', you endup mutate 'used' > > twice. > > > > I think `Master::removeOfferOperation` should be responsible to > > maintain invariants between allocator, Slave and Framework (similar to > > `Master::updateOfferOperation`). Ideally, it should not be called except > > in operation ack handler. > > > > I understand that `Master::addOfferOperation` is an exception because > > it does not mutatae allocator state (because resources might already be > > allocated in `_accept` for example), which is indeed a bit weird. > > > > But I would rather treat `addOfferOperation` as an exception, instead > > of treating `updateOfferOperation` as an exception. > > > > As a result, I still suggest we update allocator state in > > `Master::removeOfferOperation`. No need to call > > `slave->recoverResources(operation);` here because it'll be called by > > `Slave::removeOfferOperation`.
We now possibly recover resources from pending, non-speculated offer operation in `Master::removeOfferOperation` in an updated https://reviews.apache.org/r/63842/. I also simplified the `updateSlave` handler here to not explictly invoke `Slave::recoverResources` anymore and instead delegate that to an adjusted `Slave::removeOfferOperation` which I also adjusted in https://reviews.apache.org/r/63842/. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63732/#review192214 ----------------------------------------------------------- On Nov. 30, 2017, 5 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63732/ > ----------------------------------------------------------- > > (Updated Nov. 30, 2017, 5 p.m.) > > > Review request for mesos, Jie Yu and Jan Schlicht. > > > Bugs: MESOS-8207 > https://issues.apache.org/jira/browse/MESOS-8207 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/master/master.cpp fadc78b2ca5d46b8cc12a794b428753aa79ac095 > > > Diff: https://reviews.apache.org/r/63732/diff/10/ > > > Testing > ------- > > `make check`, tested as part of https://reviews.apache.org/r/63843/. > > This patch requires `protobuf::isSpeculativeOperation` from > https://reviews.apache.org/r/63751/ which is _not_ part of this review chain > (to avoid multiple dependent RRs). > > > Thanks, > > Benjamin Bannier > >
