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



Mostly minor comments all around. The two major ones include concerns around 
not killing the child container in the EOF case and if we should add the test 
at all in its present form?


src/slave/http.cpp (line 1982)
<https://reviews.apache.org/r/54196/#comment228224>

    Why not be explicit here and set it to `ContainerClass::DEFAULT` like we do 
for the corresponding session one?



src/slave/http.cpp (lines 2129 - 2130)
<https://reviews.apache.org/r/54196/#comment228245>

    How about?
    ```cpp
    // Helper that reads data from `writer` and writes to `reader`.
    // Returns a failed future if there are any errors reading or writing. The 
future is satisfied when we get a EOF.
    ```



src/slave/http.cpp (line 2136)
<https://reviews.apache.org/r/54196/#comment228246>

    4 space indent



src/slave/http.cpp (line 2178)
<https://reviews.apache.org/r/54196/#comment228240>

    Nit: comma after `ok`



src/slave/http.cpp (line 2181)
<https://reviews.apache.org/r/54196/#comment228236>

    Can you move `slave->self()` to the next line and the next line would fit 
with it.
    
    ```cpp
    .then(defer(
        slave->self(), [=]...))
    ```



src/slave/http.cpp (line 2187)
<https://reviews.apache.org/r/54196/#comment228237>

    Why do we need this logging?



src/slave/http.cpp (line 2196)
<https://reviews.apache.org/r/54196/#comment228239>

    s/client we/client, we



src/slave/http.cpp (line 2197)
<https://reviews.apache.org/r/54196/#comment228238>

    Nit: s/goes way/is interrupted



src/slave/http.cpp (line 2213)
<https://reviews.apache.org/r/54196/#comment228247>

    s/write it to/write to



src/slave/http.cpp (line 2214)
<https://reviews.apache.org/r/54196/#comment228248>

    s/Response/response



src/slave/http.cpp (line 2229)
<https://reviews.apache.org/r/54196/#comment228232>

    Can we resolve this TODO? This can happen often if the launched container 
as part of session terminates. This would result in an EOF. 
    
    You might want to sync up with Kevin regarding other cases when this can 
happen.



src/slave/http.cpp (lines 2234 - 2243)
<https://reviews.apache.org/r/54196/#comment228229>

    hmm, can we create a lambda destroy and just invoke it with a `message` 
argument rather then duplicating it here and else where?



src/slave/http.cpp (line 2249)
<https://reviews.apache.org/r/54196/#comment228250>

    Kill the extraneous space at the end since you have one extra one on the 
next line



src/tests/api_tests.cpp (line 3623)
<https://reviews.apache.org/r/54196/#comment228244>

    hmm, this test would break the moment Jie commits Kevin's changes to add 
support to the containerizer for launching debug containers?



src/tests/api_tests.cpp (line 3679)
<https://reviews.apache.org/r/54196/#comment228243>

    Use this directly in L3685. You don't need the accept header here due to 
the response being `InternalServerError`.


- Anand Mazumdar


On Dec. 1, 2016, 6:46 a.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54196/
> -----------------------------------------------------------
> 
> (Updated Dec. 1, 2016, 6:46 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-6471
>     https://issues.apache.org/jira/browse/MESOS-6471
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> In addition to launching the nested container the API handler
> ensures that the container is destroyed if the connection breaks.
> 
> 
> Diffs
> -----
> 
>   src/slave/http.cpp 87189dd6e2e099cb74faabd3ad26aaca11e5cef2 
>   src/slave/slave.hpp 05865a09724f7a46e9c06b02e59779513fc532aa 
>   src/slave/validation.cpp 46e84361244394196ee5d431c1da34686d6c938a 
>   src/tests/api_tests.cpp b2a70f3174824d3cb241a098d96e4d63a390f8bc 
> 
> Diff: https://reviews.apache.org/r/54196/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> Added a basic test for now that tests the failure case. Will be adding more 
> tests in subsequent reviews.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Reply via email to