> On March 30, 2015, 6:40 p.m., Jie Yu wrote: > > 3rdparty/libprocess/include/process/http.hpp, line 587 > > <https://reviews.apache.org/r/32351/diff/2/?file=903535#file903535line587> > > > > Can you put a TODO here you wrote in the description of this review: > > ``` > > Long term I'd like to just have a http::request(..., Request) function > > rather than the overloads. > > ```
There is an existing TODO above all of these: ``` // TODO(bmahler): Consolidate these functions into a single // http::request function that takes a 'Request' object. ``` > On March 30, 2015, 6:40 p.m., Jie Yu wrote: > > 3rdparty/libprocess/src/http.cpp, line 659 > > <https://reviews.apache.org/r/32351/diff/2/?file=903536#file903536line659> > > > > Can you handle the DISCARDED case as well (instead of a CHECK). We > > never know if the impl of the socket recv will change or not. > > ``` > > if (!data.isReady()) { > > // XXX > > responses = decoder->decode("", 0); > > } else { > > ... > > } > > ``` Sure, although I thought we wanted DISCARDED to only occur when a caller requests a discard..? > On March 30, 2015, 6:40 p.m., Jie Yu wrote: > > 3rdparty/libprocess/src/http.cpp, line 828 > > <https://reviews.apache.org/r/32351/diff/2/?file=903536#file903536line828> > > > > Why change the name to `internal::_recv`? > > > > This function tries to decode the content received from the socket. > > Using the name `internal::decode` seems to be more appropriate. This was because previously `decode` only performed decoding. Now we need to continue receiving from the socket inside both `_recv` and `__recv`. I agree it is less intuitive though, so I've renamed them to `decode` and `_decode`. Thanks! > On March 30, 2015, 6:40 p.m., Jie Yu wrote: > > 3rdparty/libprocess/src/http.cpp, lines 551-558 > > <https://reviews.apache.org/r/32351/diff/2/?file=903536#file903536line551> > > > > I would rather reorder the helpers so that I don't need to do forward > > declarations. Hm.. looking around the code, it seems like we maintain the continuation order even if it means forward declaring. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32351/#review78111 ----------------------------------------------------------- On March 23, 2015, 11 p.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32351/ > ----------------------------------------------------------- > > (Updated March 23, 2015, 11 p.m.) > > > Review request for mesos, Benjamin Hindman, Jie Yu, and Joris Van Remoortere. > > > Bugs: MESOS-2438 > https://issues.apache.org/jira/browse/MESOS-2438 > > > Repository: mesos > > > Description > ------- > > The caller must make the decision as to whether it expects to stream the > response via PIPE or get the full BODY response, otherwise they must > implement both cases, which is pretty hacky. > > So the approach here is to introduce a streaming:: namespace for this. Long > term I'd like to just have a http::request(..., Request) function rather than > the overloads. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/http.hpp > 2b366986b1f617e95bda94e07f2a0e532f5626f6 > 3rdparty/libprocess/src/http.cpp 276cecd17364989093e6eed8e97ff3a02fb0d0ef > 3rdparty/libprocess/src/tests/http_tests.cpp > 17fb092a851e128c137152fdce57e9fb10a08bf7 > > Diff: https://reviews.apache.org/r/32351/diff/ > > > Testing > ------- > > Added a test. > > > Thanks, > > Ben Mahler > >
