----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53490/#review155502 -----------------------------------------------------------
What you have looks good, I would just suggest that you also cover the case where the request body does not complete to EOF, but fails. In this case the server should not receive the request at all (if !RouteOptions.streaming) or should see the request.pipe fail (if RouteOptions.streaming). I looked through the existing tests for the response streaming, they are pretty lacking, you can blame me for that :) Also, they are a bit inconsistent with how you added the test here, so hopefully at some point we can circle back and clean up the existing tests. 3rdparty/libprocess/src/tests/http_tests.cpp (line 89) <https://reviews.apache.org/r/53490/#comment225469> streaming vs pipe (above) seems prone to confusion? Probably we should do s/pipe/streamingResponse/ and s/streaming/streamingRequest/ 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1186 - 1190) <https://reviews.apache.org/r/53490/#comment225470> There is no guarantee that the writes on the client side will correspond to the reads on the server side. This is my bad because it looks like I made this assumption in the tests I added for the response streaming, what was I thinking?? :) We have to instead just assert that the entire contents read on the server side correspond to the entire contents written on the client side. Or if we want to test more rigorously, that if I write "Hello", I will read "Hello" via 1, 2, 3, 4, or 5 read calls. For example (pseudo-code): ``` writer.write("Hello"); while (read != "Hello") { Future<string> f = reader.read(); AWAIT_READY(f); read += f.get(); ASSERT_TRUE(string("Hello").startsWith(read)); } ``` - Benjamin Mahler On Nov. 4, 2016, 5:55 p.m., Anand Mazumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53490/ > ----------------------------------------------------------- > > (Updated Nov. 4, 2016, 5:55 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-6466 > https://issues.apache.org/jira/browse/MESOS-6466 > > > Repository: mesos > > > Description > ------- > > Added a test for request streaming via the connection abstraction. > > > Diffs > ----- > > 3rdparty/libprocess/src/tests/http_tests.cpp > 533104c93dd1eaf67bf3752163d2e0cad090078d > > Diff: https://reviews.apache.org/r/53490/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Anand Mazumdar > >
