-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52135/#review150029
-----------------------------------------------------------


Fix it, then Ship it!





src/slave/http.cpp (line 1932)
<https://reviews.apache.org/r/52135/#comment217840>

    I would jsut say "Failed to launch container " to be consistent with 
destroy message below.



src/slave/http.cpp (line 1967)
<https://reviews.apache.org/r/52135/#comment217841>

    No need for quotes around containerId because they are UUID strings? you 
didn't use them in launch above.



src/slave/http.cpp (lines 1977 - 1979)
<https://reviews.apache.org/r/52135/#comment217842>

    I'm wondering if we should include the whole ContainerTermination proto in 
the response instead of just the status.  For example, the "message" and 
"reasons" might be useful if the container OOMs.
    
    Thoughts?



src/tests/api_tests.cpp (lines 3235 - 3280)
<https://reviews.apache.org/r/52135/#comment217844>

    Hmm. I would put them in their own separate tests since the setup for them 
is easy enough.



src/tests/api_tests.cpp (line 3359)
<https://reviews.apache.org/r/52135/#comment217843>

    can you add a comment on why you set these expectations here? who calls 
`containers()`?


- Vinod Kone


On Sept. 22, 2016, 6:06 a.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52135/
> -----------------------------------------------------------
> 
> (Updated Sept. 22, 2016, 6:06 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2449
>     https://issues.apache.org/jira/browse/MESOS-2449
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch adds the wiring for the *_NESTED_CONTAINER calls,
> including validation and calling into the containerizer.
> 
> 
> Diffs
> -----
> 
>   src/slave/http.cpp 0aae14583faf95fee678b3a3424c96cdba531968 
>   src/tests/api_tests.cpp 26f99f7c337fbbc5278d1b30d3d5c8f659ddf5ca 
> 
> Diff: https://reviews.apache.org/r/52135/diff/
> 
> 
> Testing
> -------
> 
> Added a test that mocks out the containerizer.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Reply via email to