----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70968/#review216215 -----------------------------------------------------------
Thanks for fixing this! Fixed logic looks good, but the commit description is vague. Can you expand the commit description to clarify what the issues were and what this patch does to fix them? (example below) src/master/allocator/mesos/hierarchical.cpp Lines 552 (patched) <https://reviews.apache.org/r/70968/#comment303320> Per the top-level comment, can you document in the commit description each issue that was fixed? This one looks like: -When a role is removed, if the framework still has resources allocated to the role we previously did not deactivate the role. (... what's the consequence of this as well?) src/master/allocator/mesos/hierarchical.cpp Lines 550-551 (original), 568-569 (patched) <https://reviews.apache.org/r/70968/#comment303319> Per the comment in the previous review, it's suprising that newUnsuppressedRoles can contain entries in removedRoles, shall we just adjust the set construction so that it does not? - Benjamin Mahler On June 27, 2019, 7:38 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70968/ > ----------------------------------------------------------- > > (Updated June 27, 2019, 7:38 p.m.) > > > Review request for mesos, Benjamin Mahler and Meng Zhu. > > > Bugs: MESOS-9870 > https://issues.apache.org/jira/browse/MESOS-9870 > > > Repository: mesos > > > Description > ------- > > This patch fixes bugs in updateFramework() in the allocator code > which can be triggered by UPDATE_FRAMEWORK or re-subscription - > see MESOS-9870. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > 26aad6778f12b99bb87c846788d6b6d60f743d8a > > > Diff: https://reviews.apache.org/r/70968/diff/1/ > > > Testing > ------- > > make check > > > Thanks, > > Andrei Sekretenko > >
