----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48563/#review137111 -----------------------------------------------------------
Fix it, then Ship it! Logically looks fine to me, although now I'm worried that we have another 100 `.then()`'s that should be deferring. Please consider my suggestions for wrapping/indentation. src/master/http.cpp (lines 1869 - 1872) <https://reviews.apache.org/r/48563/#comment202265> I don't really like this indentation style, since it makes the tuple fields seem like they're at a higher level than they are. Looking around, it seems we have a stronger preference to wrap the `[=]` onto the next line (indented 4), and sometimes even wrap the `->` onto the next line. How about: ``` return collect(frameworksApprover, tasksApprover, executorsApprover) .then(defer(master->self(), [=](const tuple<Owned<ObjectApprover>, Owned<ObjectApprover>, Owned<ObjectApprover>>& approvers) -> Response { auto state = [this, &approvers](JSON::ObjectWriter* writer) { ``` src/master/http.cpp (lines 2217 - 2222) <https://reviews.apache.org/r/48563/#comment202267> Similarly, I think this looks better if we wrap before the `[]` instead of after the `(`: ``` return frameworksApprover .then(defer(master->self(), [=](const Owned<ObjectApprover>& frameworksApprover) -> Response { auto stateSummary = [this, &frameworksApprover](JSON::ObjectWriter* writer) { Owned<ObjectApprover> frameworksApprover_ = frameworksApprover; ``` See how it keeps the lambda function definition on one line? src/master/http.cpp (lines 2662 - 2668) <https://reviews.apache.org/r/48563/#comment202268> Ditto: ``` return collect(frameworksApprover, tasksApprover, executorsApprover) .then(defer(master->self(), [=](const tuple<Owned<ObjectApprover>, Owned<ObjectApprover>, Owned<ObjectApprover>>& approvers) -> Response { // Get approver from tuple. Owned<ObjectApprover> frameworksApprover; ``` - Adam B On June 10, 2016, 9:28 p.m., Joerg Schad wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48563/ > ----------------------------------------------------------- > > (Updated June 10, 2016, 9:28 p.m.) > > > Review request for mesos and Adam B. > > > Bugs: MESOS-5587 > https://issues.apache.org/jira/browse/MESOS-5587 > > > Repository: mesos > > > Description > ------- > > Previously the continuation followed via `.then([=]`, > which potentially executes the continuation on a different > process. This patch fixes this behavior and avoids race > conditions between the master process changing the > related data-structures and another process reading them. > > > Diffs > ----- > > src/master/http.cpp 4b2d1386e1ecb447b597a432f9df9adaa5c3aa37 > > Diff: https://reviews.apache.org/r/48563/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Joerg Schad > >
