----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63732/#review192214 -----------------------------------------------------------
Fix it, then Ship it! src/master/master.cpp Lines 7426 (patched) <https://reviews.apache.org/r/63732/#comment270252> 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`. - Jie Yu On Nov. 29, 2017, 4:27 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63732/ > ----------------------------------------------------------- > > (Updated Nov. 29, 2017, 4:27 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 700e12433b0b66efc3f5dd296711c0f203a13144 > > > Diff: https://reviews.apache.org/r/63732/diff/9/ > > > 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 > >
