----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53994/#review156978 -----------------------------------------------------------
Looks good. Mostly comments around getting rid of the `_api()` handler/doing redundant input handling in one place in the `api()` handler itself. Here is the diff based on my comments later: https://gist.github.com/hatred/eaff00183041f00613bd6e83ed393813 (Feel free to apply it if you agree with the comments, there might be some style issues though) src/slave/http.cpp (line 313) <https://reviews.apache.org/r/53994/#comment227353> I would just use `contentType_` as this is too verbose. src/slave/http.cpp (lines 324 - 325) <https://reviews.apache.org/r/53994/#comment227354> 4 space indent src/slave/http.cpp (lines 330 - 345) <https://reviews.apache.org/r/53994/#comment227355> hmm, I know we talked about this offline but I still don't see much utility in having this method. We should be able to move this in the `api()` handler itself. src/slave/http.cpp (line 334) <https://reviews.apache.org/r/53994/#comment227356> We don't need to copy the entire request to get a non const reader. ```cpp Pipe::Reader reader = request.reader.get(); // Remove const. ``` src/slave/http.cpp (lines 348 - 351) <https://reviews.apache.org/r/53994/#comment227357> We should do the `contentType`, `acceptType` parsing at one place in the `api()` handler itself. It feels a bit weird that we need to get the `Content-Type` headers twice from the `Request` object once in the `api()` handler and then again here. As per my earlier comment, if we get rid of the `_api()`, a possible signature for this method can be: ```cpp Future<Response> Slave::Http::__api( ContentType contentType, ContentType acceptType, const string& body, const Option<string>& principal) const ``` Note that this would mean that you can't pass the `Request` object as it is to the `attachContainerOutput` handler later. You would need to create a new `Request` object. It is no longer acting like a proxy handler anyways due to it invoking `transform()`. src/slave/http.cpp (lines 356 - 373) <https://reviews.apache.org/r/53994/#comment227358> This can be moved to the `api()` handler itself. - Anand Mazumdar On Nov. 26, 2016, 5:42 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53994/ > ----------------------------------------------------------- > > (Updated Nov. 26, 2016, 5:42 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 > ------- > > Note that this change only updates the handler to correctly > handle non-streaming calls given it receives `PIPE` requests. > Handling streaming calls will come later. > > > Diffs > ----- > > src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f > src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb > src/slave/slave.cpp 521f08d59cd78f9089d58cd3294f0ee4a099cd7f > > Diff: https://reviews.apache.org/r/53994/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
