> On Nov. 6, 2015, 5:23 p.m., Ben Mahler wrote: > > src/common/protobuf_utils.cpp, lines 178-187 > > <https://reviews.apache.org/r/40036/diff/1/?file=1118429#file1118429line178> > > > > You don't need the 'if' with the way you've structured your loop, I > > assume it's just here from copy paste?
Yup, thanks for catching this. > On Nov. 6, 2015, 5:23 p.m., Ben Mahler wrote: > > src/common/protobuf_utils.cpp, line 182 > > <https://reviews.apache.org/r/40036/diff/1/?file=1118429#file1118429line182> > > > > Consider: > > > > ``` > > for (auto status = task.statuses().rbegin(); status != > > task.statuses().rend(); ++status) > > ``` > > > > Ideally we could do: > > > > ``` > > foreach (const TaskStatus& status, reverse(task.statuses())) > > ``` > > > > If you want to avoid the indexing math. I chose the former ... looks like `foreach (..., reverse())` isn't used anywhere in the codebase (even `reverse()` alone isn't being used) and so wasn't sure if I should do it. (It would have also required `#include <algorithm>`). I wouldn't mind changing it though. Any preferences? > On Nov. 6, 2015, 5:23 p.m., Ben Mahler wrote: > > src/tests/master_tests.cpp, line 3371 > > <https://reviews.apache.org/r/40036/diff/1/?file=1118431#file1118431line3371> > > > > The convention here would be `status2` or something like > > `reconciliationStatus`. I changed it to `explicitReconciliationStatus` to make it more explicit. Same for implicit. > On Nov. 6, 2015, 5:23 p.m., Ben Mahler wrote: > > src/tests/master_tests.cpp, lines 3375-3379 > > <https://reviews.apache.org/r/40036/diff/1/?file=1118431#file1118431line3375> > > > > You can avoid the vector here by passing in an initializer list into > > your reconcileTasks call. Fixed. > On Nov. 6, 2015, 5:23 p.m., Ben Mahler wrote: > > src/common/protobuf_utils.cpp, line 190 > > <https://reviews.apache.org/r/40036/diff/1/?file=1118429#file1118429line190> > > > > two newlines here :) Fixed. > On Nov. 6, 2015, 5:23 p.m., Ben Mahler wrote: > > src/tests/master_tests.cpp, line 3396 > > <https://reviews.apache.org/r/40036/diff/1/?file=1118431#file1118431line3396> > > > > Ditto here. Fixed. - Kapil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40036/#review105513 ----------------------------------------------------------- On Nov. 6, 2015, 8:47 p.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40036/ > ----------------------------------------------------------- > > (Updated Nov. 6, 2015, 8:47 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-3845 > https://issues.apache.org/jira/browse/MESOS-3845 > > > Repository: mesos > > > Description > ------- > > Along the lines of health checks, the container_status should also be sent > inside reconciliation updates. > > > Diffs > ----- > > src/common/protobuf_utils.hpp 44a2b1d3bab227294f5c5854acd87b7bd75ef589 > src/common/protobuf_utils.cpp b3c2d1183647ec1961f5d404355d08a5ce1d42a0 > src/master/master.cpp 25b94c8b41f65599327e0a0459ba86c6078895a8 > src/tests/master_tests.cpp 856440559de7d58be5bcf2ab6be911b5c67001cd > > Diff: https://reviews.apache.org/r/40036/diff/ > > > Testing > ------- > > `make check` with updated `MasterTest.TaskStatusContainerStatus` test. > > > Thanks, > > Kapil Arya > >
