mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 53490: Added a test for request streaming via the connection abstraction.
Date Wed, 09 Nov 2016 21:50:44 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message