----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71436/#review217568 -----------------------------------------------------------
Fix it, then Ship it! src/master/http.cpp Lines 4332-4339 (original), 4332-4339 (patched) <https://reviews.apache.org/r/71436/#comment304838> It's unfortunate that this hack leads to rescindOffer to have a Filters arg. As one would expect why could master initiated rescind needs to have a filter. src/master/master.hpp Lines 992 (patched) <https://reviews.apache.org/r/71436/#comment304839> ``` // Both methods recover the resources in the allocator and remove the offer // in the master. `rescindOffer` further notifies the framework about the // rescind. // // NOTE: the filters field in `rescindOffer` is needed as a workaround for // the race between the master and allocator. This happens when the master // tries to freeing up resources to satisfy operator initiated operations. void rescindOffer(Offer* offer, const Option<Filters>& filters = None()); void discardOffer(Offer* offer, const Option<Filters>& filters = None()); ``` src/master/master.cpp Line 3357 (original), 3352 (patched) <https://reviews.apache.org/r/71436/#comment304837> Looking at the call site, this `rescind` parameter is redundant, we could just look at framework->active(), this saves one parameter and also saves me from wondering what makes a `resind` true. src/master/master.cpp Line 4914 (original), 4895 (patched) <https://reviews.apache.org/r/71436/#comment304840> s/discardExistingOffers/discardOffers/ Also, can you make the capture cleaner? ``` auto discardOffers = [](const Repeated<OfferId>& ...) { }; discardOffers(accept.offer_ids()); ``` src/master/master.cpp Line 8849 (original), 8811 (patched) <https://reviews.apache.org/r/71436/#comment304841> LOG should be "Rescinding"? src/master/master.cpp Line 8910 (original), 8860 (patched) <https://reviews.apache.org/r/71436/#comment304842> s/Remove and rescind/Rescind/ src/master/master.cpp Lines 12701-12702 (original), 12656-12657 (patched) <https://reviews.apache.org/r/71436/#comment304843> consider avoid the duplicate lookup by letting _removeOffer take a framework arg. - Meng Zhu On Sept. 4, 2019, 10:54 a.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71436/ > ----------------------------------------------------------- > > (Updated Sept. 4, 2019, 10:54 a.m.) > > > Review request for mesos, Benjamin Mahler and Meng Zhu. > > > Bugs: MESOS-1452 and MESOS-9949 > https://issues.apache.org/jira/browse/MESOS-1452 > https://issues.apache.org/jira/browse/MESOS-9949 > > > Repository: mesos > > > Description > ------- > > This patch adds helper methods `Master::rescindOffer()` / > `Master::discardOffer()` that recover offer's resources in the allocator > and remove the offer, and replaces paired calls of `removeOffer()` + > `allocator->recoverResources()` with these helpers. > > > Diffs > ----- > > src/master/http.cpp 0987d933c03b059c70f0b7c6231df73b8b4d228f > src/master/master.hpp 19c17821d2b8111ebe8e1429a5b4a8423357fdd6 > src/master/master.cpp f00906ef2d33920f23127a74ed141fff9d32865b > src/master/quota_handler.cpp f28eb27964afd2c8bfa0bf7a3ab8246442812f2a > src/master/weights_handler.cpp dfb6f06449f85e7acb156cac0ff79cadcc712255 > > > Diff: https://reviews.apache.org/r/71436/diff/1/ > > > Testing > ------- > > make check, 10 times > > > Thanks, > > Andrei Sekretenko > >
