> On March 18, 2015, 11:49 p.m., Jie Yu wrote: > > 3rdparty/libprocess/include/process/http.hpp, lines 167-186 > > <https://reviews.apache.org/r/31930/diff/2/?file=896914#file896914line167> > > > > Instead of keeping a reference to Data, wouldn't be simpler and more > > straitforward to keep a Pipe instance (as Pipe is already copyable) here? > > You can avoid the forward declarations as well. > > > > ``` > > class Pipe > > { > > public: > > class Reader > > { > > public: > > ... > > > > private: > > // Necessary for accessing pipe.data. > > friend class Pipe; > > > > Reader(const Pipe& _pipe) : pipe(_pipe) {} > > > > Pipe pipe; > > }; > > }; > > ```
Chatted with Jie, this leads to an incomplete type issue during compilation and it's fairly clunky to work around it (need to forward declare Reader/Writer in Pipe, and declare Reader/Writer fully outside Pipe). > On March 18, 2015, 11:49 p.m., Jie Yu wrote: > > 3rdparty/libprocess/include/process/http.hpp, line 205 > > <https://reviews.apache.org/r/31930/diff/2/?file=896914#file896914line205> > > > > Are you exposing this function only for testing? If yes, please add a > > comment. Chatted with Jie, this is necessary for the pipe writer to detect a disconnection (e.g. master streaming events to a framework, framework disconnects). > On March 18, 2015, 11:49 p.m., Jie Yu wrote: > > 3rdparty/libprocess/include/process/http.hpp, lines 208-209 > > <https://reviews.apache.org/r/31930/diff/2/?file=896914#file896914line208> > > > > Ditto. Per above we'll store Data to avoid the incomplete type issues. > On March 18, 2015, 11:49 p.m., Jie Yu wrote: > > 3rdparty/libprocess/src/http.cpp, lines 44-46 > > <https://reviews.apache.org/r/31930/diff/2/?file=896915#file896915line44> > > > > This can be simplified as: > > ``` > > return Pipe::Reader(*this); > > ``` I've added a constructor to avoid the default initialization issue, thanks! > On March 18, 2015, 11:49 p.m., Jie Yu wrote: > > 3rdparty/libprocess/src/http.cpp, line 101 > > <https://reviews.apache.org/r/31930/diff/2/?file=896915#file896915line101> > > > > Can you add an extra parathesis here. I am not super familar with the > > operator priorities:) Jie and I chatted about this, we probably wouldn't add parens if this was +, >, !=, etc, and we already have this without parens in other places in the code. > On March 18, 2015, 11:49 p.m., Jie Yu wrote: > > 3rdparty/libprocess/src/process.cpp, lines 1149-1151 > > <https://reviews.apache.org/r/31930/diff/2/?file=896916#file896916line1149> > > > > Does this comment still apply? Looks like we cannot write empty string. > > Maybe a CHECK? Chatted with Jie, this is the clunkiness that is induced from introducing Option<string> per benh and joris' comments. Callers **still** have to think about the empty string case because the type can express it. The alternative is we add an implicit invariant that empty reads are never returned, which seems pretty hacky (all callers will CHECK against it). Will keep this as a special case for now. But we may want to circle back to Future<string>, as it seems clearer now that we'll probably not want to use a typed Stream<T> abstraction for this case. Rather, for raw byte streams we probably want to stick with a specialized abstraction (like Pipe). - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31930/#review76931 ----------------------------------------------------------- On March 16, 2015, 11:49 p.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31930/ > ----------------------------------------------------------- > > (Updated March 16, 2015, 11:49 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 > ------- > > See MESOS-2438 for the motivation. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/http.hpp > 10143fdbd5eb43f968c41957a2335f96a097d82e > 3rdparty/libprocess/src/http.cpp 7c0cee404645e1dd1a9253f85883aefa29a5f999 > 3rdparty/libprocess/src/process.cpp > e7b029ba97e640c2102548c190ba62b30602f43d > 3rdparty/libprocess/src/tests/http_tests.cpp > 800752a57230d7768d3d741fef87f6283757e424 > > Diff: https://reviews.apache.org/r/31930/diff/ > > > Testing > ------- > > * added tests > * make check > > > Thanks, > > Ben Mahler > >
