mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 34654: Send docker inspect output with TaskStatus data.
Date Wed, 27 May 2015 17:50:27 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message