mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anand Mazumdar <an...@apache.org>
Subject Re: Review Request 53995: Added API handler for ATTACH_CONTAINER_OUTPUT.
Date Mon, 28 Nov 2016 23:45:38 GMT

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



Looks good! Just some minor comments around style/questions around implementation.


src/internal/evolve.hpp (line 80)
<https://reviews.apache.org/r/53995/#comment227596>

    Can you move this method after L78 to sort them by return type as we did for the other
overloads in this file?
    
    You would also need to move the method in the .cpp file



src/slave/http.cpp (line 89)
<https://reviews.apache.org/r/53995/#comment227597>

    Move this above L88?



src/slave/http.cpp (line 2119)
<https://reviews.apache.org/r/53995/#comment227599>

    4 space indent here



src/slave/http.cpp (line 2125)
<https://reviews.apache.org/r/53995/#comment227600>

    Nit: s/'HOST'/The 'HOST'



src/slave/http.cpp (lines 2131 - 2132)
<https://reviews.apache.org/r/53995/#comment227598>

    hmm, it's somewhat weird that we set it to `/api`. Why not `/switchboard` or anything
else.
    
    I would just set it to `/` i.e., the server root since currently i.e. the only path we
support on the server. We can then make the switchboard implementation validate it. Is there
an issue if we set it to `/`?



src/slave/http.cpp (line 2138)
<https://reviews.apache.org/r/53995/#comment227601>

    Can you be explicit here? (You don't need access to `this` here and it can lead to subtle
bugs in the future)



src/slave/http.cpp (line 2143)
<https://reviews.apache.org/r/53995/#comment227602>

    Quotes before `ProcessIO`



src/slave/http.cpp (lines 2145 - 2148)
<https://reviews.apache.org/r/53995/#comment227605>

    You might want to reorder these statements as:
    
    ```cpp
              Pipe pipe;
              Pipe::Writer writer = pipe.writer();
              
              OK ok;
              ok.reader = pipe.reader();
              
    ```



src/slave/http.cpp (line 2160)
<https://reviews.apache.org/r/53995/#comment227607>

    Do you want to capture everything here?



src/slave/http.cpp (line 2168)
<https://reviews.apache.org/r/53995/#comment227608>

    hmm, you would need to capture `Connection` here in this lambda. I don't quite understand
how it works if you don't do that i.e., the destructor of `Connection` would be called since
there is no reference to it remaining that would in turn result in the receiving side of the
socket closing i.e., it won't accept any more streaming responses!



src/tests/api_tests.cpp (line 80)
<https://reviews.apache.org/r/53995/#comment227609>

    Why do you need this using declaration and the corresponding header?



src/tests/api_tests.cpp 
<https://reviews.apache.org/r/53995/#comment227611>

    hmm, why did you remove this using declaration?



src/tests/containerizer/mock_containerizer.hpp (lines 71 - 73)
<https://reviews.apache.org/r/53995/#comment227610>

    hmm, not sure why did you need the change in this review? You don't seem to be using the
`MockContainerizer` in your test?


- Anand Mazumdar


On Nov. 28, 2016, 10:50 p.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53995/
> -----------------------------------------------------------
> 
> (Updated Nov. 28, 2016, 10:50 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6473
>     https://issues.apache.org/jira/browse/MESOS-6473
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added API handler for ATTACH_CONTAINER_OUTPUT.
> 
> 
> Diffs
> -----
> 
>   src/internal/evolve.hpp e8b03c9f04b8a835f35ce7cb6cc44e3ecd799422 
>   src/internal/evolve.cpp 23a515c0aa8c31e0789fab84830dae32574604bc 
>   src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f 
>   src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb 
>   src/slave/validation.cpp b3d8bcd0f0855d2a0a96821900d996ba86e11578 
>   src/tests/api_tests.cpp 8889e7807ecead736eaac3910332e86d594e8cec 
>   src/tests/containerizer/mock_containerizer.hpp 7a30b8307b93b7bf549efb52d72367f652d0d95a

> 
> Diff: https://reviews.apache.org/r/53995/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>


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