> On June 28, 2019, 3:38 a.m., Greg Mann wrote: > > src/slave/slave.cpp > > Lines 9783-9795 (patched) > > <https://reviews.apache.org/r/70959/diff/3/?file=2152676#file2152676line9783> > > > > I don't think it's sufficient to check `framework->idle()`, since > > `Framework::idle()` does not check `operations` at all. Could this block be > > replaced by: > > > > ``` > > if (!frameworks.empty() || !operations.empty()) { > > return; > > } > > ``` > > ?
The first check this expression performs is actually `operations.empty()`, but this is a good point about `idle`: one scenario we call this function from is for `idle` frameworks which are `removeFramework`'d, i.e., it is sufficient to check that `frameworks.empty()`. I have rewritten the code as suggested by you, but kept the temporary for readability. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70959/#review216218 ----------------------------------------------------------- On June 28, 2019, 11:27 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70959/ > ----------------------------------------------------------- > > (Updated June 28, 2019, 11:27 a.m.) > > > Review request for mesos, Greg Mann and Joseph Wu. > > > Bugs: MESOS-9860 > https://issues.apache.org/jira/browse/MESOS-9860 > > > Repository: mesos > > > Description > ------- > > Once a draining agent has neither frameworks with pending tasks nor any > executors with either queued or launched tasks it has finished draining. > This patch adds handling of that case which clears both the in-memory > and persisted drain configuration. > > > Diffs > ----- > > src/slave/slave.hpp 6954f53ff1531b9fcb688ef76acddf6a3d849a41 > src/slave/slave.cpp 30039b0857a4d85b4b96fa95d7f8724d57cdec6e > > > Diff: https://reviews.apache.org/r/70959/diff/4/ > > > Testing > ------- > > `make check` > > tested as part of https://reviews.apache.org/r/70960/ > > > Thanks, > > Benjamin Bannier > >
