----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34654/#review85390 -----------------------------------------------------------
Ship it! This looks good but I'd like to can clean up the 'output' part and just use the JSON that gets passed in. src/docker/docker.hpp <https://reviews.apache.org/r/34654/#comment136944> My expectation based on our design discussions was that we were going to save just the JSON and then send the stringified version of the JSON back in the 'TaskStatus.data'. While saving the output from 'docker inspect' effectively does the same thing it seems weird to pass two parameters to the create function that represent the same data. Why not just hold on to the JSON as a field 'json' here and then stringify that in docker/executor.cpp? The thing that was especially wierd to me about the current code is that it relies on the invariant in our 'docker inspect' that the output only has an array of 1 JSON object which isn't captured in the code or as an invariant and if ever changed could cause a subtle bug. Maybe I'm missing something though? src/docker/executor.cpp <https://reviews.apache.org/r/34654/#comment136940> I'm assuming that you're just delaying sending the TASK_RUNNING until 'inspect' properly returns? Can you comment as much? I don't think it's obvious that calling inspect satisfies this requirement. src/docker/executor.cpp <https://reviews.apache.org/r/34654/#comment136941> Why back to MergeFrom instead of CopyFrom? src/docker/executor.cpp <https://reviews.apache.org/r/34654/#comment136938> You should be able to drop the parameter all together, in the lambdas below as well. Can you give that a shot and let me know if it doesn't work please? src/docker/executor.cpp <https://reviews.apache.org/r/34654/#comment136937> Can we capture this as a constant please? src/docker/executor.cpp <https://reviews.apache.org/r/34654/#comment136934> -2 on each line here. - Benjamin Hindman On May 26, 2015, 6:14 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34654/ > ----------------------------------------------------------- > > (Updated May 26, 2015, 6:14 p.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: MESOS-2368 > https://issues.apache.org/jira/browse/MESOS-2368 > > > Repository: mesos > > > Description > ------- > > Send docker inspect output with TaskStatus data. > > > Diffs > ----- > > src/docker/docker.hpp d06c73a > src/docker/docker.cpp ee74da5 > src/docker/executor.cpp 075c6b5 > src/tests/docker_containerizer_tests.cpp 7524803 > > Diff: https://reviews.apache.org/r/34654/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
