----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53487/#review156500 -----------------------------------------------------------
Fix it, then Ship it! The structure of visit looks better now, thanks! 3rdparty/libprocess/src/process.cpp (line 672) <https://reviews.apache.org/r/53487/#comment226674> `const Owned&` seems to suggest that this doesn't take ownership, perhaps take `Owned`? 3rdparty/libprocess/src/process.cpp (lines 2712 - 2713) <https://reviews.apache.org/r/53487/#comment226675> Can you clarify for the reader why socket manager finalization is tied to this continuation being invoked? 3rdparty/libprocess/src/process.cpp (lines 2716 - 2717) <https://reviews.apache.org/r/53487/#comment226677> Hm.. seems this is where we need a comment about how we're guaranteed that the socket manager is valid inside this continuation? 3rdparty/libprocess/src/process.cpp (lines 2725 - 2726) <https://reviews.apache.org/r/53487/#comment226676> Could you include the open and closing quotes on the same line? Also, any reason not to use the typical ": " based approach for logging the error? ``` VLOG(1) << "Returning '" << response.status << "'" << " for '" << request->url.path << "'" << ": " << response.body << ""; ``` 3rdparty/libprocess/src/process.cpp (line 3617) <https://reviews.apache.org/r/53487/#comment226678> Why the copy? Do you want a const reference here? 3rdparty/libprocess/src/process.cpp (lines 3648 - 3673) <https://reviews.apache.org/r/53487/#comment226680> Can you add a TODO (in a separate patch) that we should handle discarded responses by not processing the request? As it stands, if the event is in the queue and the caller disconnects, we will still process the request here and deliver it to the route. 3rdparty/libprocess/src/process.cpp (line 3665) <https://reviews.apache.org/r/53487/#comment226681> No need for "due to" here? Seems inconsistent with our other error logging style. 3rdparty/libprocess/src/process.cpp (line 3669) <https://reviews.apache.org/r/53487/#comment226682> No need for the return. 3rdparty/libprocess/src/process.cpp (lines 3676 - 3677) <https://reviews.apache.org/r/53487/#comment226683> Maybe a TODO like: ``` TODO(anandm): Is this an error? ``` Since I'm not sure if we should silently allow it. 3rdparty/libprocess/src/process.cpp (line 3705) <https://reviews.apache.org/r/53487/#comment226685> Not your change, but using associate here seems confusing, since it is intended for association with another Future. It happens to compile because it is implicitly constructs a `Future<Response>` from the `Response`. So: ``` event.response->set(response); ``` Would be clearer. Feel free to make this cleanup (and the other s/associate/set/ suggestion) in a patch pulled out from this one. 3rdparty/libprocess/src/process.cpp (line 3713) <https://reviews.apache.org/r/53487/#comment226684> Using associate here seems a little odd, since it is intended for association with another Future. It compiles because it is implicitly constructure a `Future<Response>` from `NotFound()`. So: ``` event.response->set(NotFound()); ``` Would be clearer. Feel free to make this cleanup (and the other s/associate/set/ suggestion) in a patch pulled out from this one. 3rdparty/libprocess/src/process.cpp (line 3720) <https://reviews.apache.org/r/53487/#comment226679> Seems to indicate that the ownership resides in the caller, as if you did: ``` void function(const unique_ptr& p) ``` Whereas the following would seem to suggest passing ownership: ``` void function(unique_ptr&& p) void function(unique_ptr p) ``` Also, in these latter options, you can move the pointer into a capture (not yet in C++11), whereas you couldn't with the first option. - Benjamin Mahler On Nov. 21, 2016, 7:25 p.m., Anand Mazumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53487/ > ----------------------------------------------------------- > > (Updated Nov. 21, 2016, 7:25 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-6466 > https://issues.apache.org/jira/browse/MESOS-6466 > > > Repository: mesos > > > Description > ------- > > Old libprocess style messages and routes not supporting request > streaming read the body from the piped reader. Otherwise, the > request is forwarded to the handler when the route supports > streaming. > > Review: https://reviews.apache.org/r/53487/ > > > Diffs > ----- > > 3rdparty/libprocess/include/process/process.hpp > f389d3d3b671e301a7ac911ad87ab13289e8c82f > 3rdparty/libprocess/src/process.cpp > ab2b5a9d38a3001d6a5daa1807fecb630c4b154d > > Diff: https://reviews.apache.org/r/53487/diff/ > > > Testing > ------- > > make check (Tests are added in https://reviews.apache.org/r/53490 i.e., after > we add support to the `Connection` abstraction for request streaming) > > > Thanks, > > Anand Mazumdar > >
