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