----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53487/#review155745 -----------------------------------------------------------
Looking good! Just a few minor things left. 3rdparty/libprocess/src/process.cpp (lines 680 - 681) <https://reviews.apache.org/r/53487/#comment225821> Don't you need to change the type to BODY here as well? 3rdparty/libprocess/src/process.cpp (lines 708 - 722) <https://reviews.apache.org/r/53487/#comment225824> Now that we're returning Future<Message*> we can capture failures explicitly as a failed future, so that the caller doesn't have to consider two failure cases. In other words, if this returns a successful future, the pointer will be non-null. (See my comment below for how that simplifies the call-site). 3rdparty/libprocess/src/process.cpp (lines 2710 - 2713) <https://reviews.apache.org/r/53487/#comment225822> It looks like this may be introducing another problematic path for libprocess finalization, in that your continuation may run after libprocess is finalized and it expects 'this' to be valid. It would be great to check with Joesph Wu about this path being introduced since he has worked on libprocess finalization, and see if he has any input. 3rdparty/libprocess/src/process.cpp (lines 2732 - 2743) <https://reviews.apache.org/r/53487/#comment225823> The intention of my previous suggestion to return Future<Message*> was to eliminate the possibility of nullptr in favor of just returning a failed future for this case. Then you only need to deal with one error path here and you can just do the following: ``` Message* message = CHECK_NOTNULL(future.get()); ``` 3rdparty/libprocess/src/process.cpp (line 3683) <https://reviews.apache.org/r/53487/#comment225825> TODO to avoid copying the response? Also, mind using associate? We have set as an alias but I believe most code calls associate for associating with another future since it's a bit more specific. 3rdparty/libprocess/src/process.cpp (lines 3683 - 3684) <https://reviews.apache.org/r/53487/#comment225826> It seems problematic to delay the association until the response transitions out of pending, as this will prevent discard propagation. (e.g. the socket disconnects and libprocess discards the response, the discard won't propagate into the handler). Can you instead perform the association as soon as you've invoked the handlers, before waiting for the response to transition? - Benjamin Mahler On Nov. 11, 2016, 7:57 p.m., Anand Mazumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53487/ > ----------------------------------------------------------- > > (Updated Nov. 11, 2016, 7:57 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. > > > 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 > >
