> On Oct. 11, 2012, 10 p.m., Ben Mahler wrote:
> > third_party/libprocess/src/process.cpp, line 1625
> > <https://reviews.apache.org/r/7544/diff/2/?file=175831#file175831line1625>
> >
> >     Why the extra parens?

This was code copied from this file, and I kept them for consistency.


> On Oct. 11, 2012, 10 p.m., Ben Mahler wrote:
> > third_party/libprocess/src/process.cpp, line 1632
> > <https://reviews.apache.org/r/7544/diff/2/?file=175831#file175831line1632>
> >
> >     The current loop logic is pretty tricky, seems this could be simplified:
> >     
> >     1. s/break/return (since finished=false if this break happens)
> >     2. s/finished=true/break
> >     3. s/while(!finished)/while(true) (adding a comment above the loop as 
> > to the fact that this is a read loop) 
> >     3. Kill the finished boolean entirely.
> >     4. Update comments as necessary.
> >     
> >     Let me know what you think, took me a while to grok this function 
> > perfectly.

This by itself actually introduces a bug (replacing 'finished=true' with 
'break' misses sending the actual data). Either way this stuff is low-level and 
complicated (which is why it's done here and not everywhere else. However, it 
currently matches the other code in this file, so I'm going to keep it like 
this for now.


> On Oct. 11, 2012, 10 p.m., Ben Mahler wrote:
> > third_party/libprocess/src/process.cpp, line 1642
> > <https://reviews.apache.org/r/7544/diff/2/?file=175831#file175831line1642>
> >
> >     Is the final CRLF of the Chunked-Body (after the trailer), getting 
> > included elsewhere? Seems this just includes the
> >     
> >     0CRLF // last chunk size
> >     CRLF // last chunk data
> >     // no chunked-body CRLF?

No trailer, so no more CRLF is needed.


> On Oct. 11, 2012, 10 p.m., Ben Mahler wrote:
> > third_party/libprocess/src/process.cpp, line 1624
> > <https://reviews.apache.org/r/7544/diff/2/?file=175831#file175831line1624>
> >
> >     So an async read is effectively taking place due to the io::poll here 
> > when the read would block?
> >     
> >     I guess io::read is not applicable here as a replacement?

Doing io::read would be cleaner, but less efficient. The current implementation 
of io::read always blocks through io::poll, and if we can refrain from blocking 
we'll get much better performance.


- Benjamin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7544/#review12357
-----------------------------------------------------------


On Oct. 11, 2012, 8:57 p.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7544/
> -----------------------------------------------------------
> 
> (Updated Oct. 11, 2012, 8:57 p.m.)
> 
> 
> Review request for mesos, Vinod Kone and Ben Mahler.
> 
> 
> Description
> -------
> 
> Added streaming HTTP responses (via pipes).
> 
> Review: https://reviews.apache.org/r/7544
> 
> 
> Diffs
> -----
> 
>   third_party/libprocess/src/process.cpp 
> e887feb1070cdd03a6d81b8f798145ed8bda7b5c 
>   third_party/libprocess/src/tests.cpp 
> 9cae36aede41bc1e6bb966122c581cff97483717 
> 
> Diff: https://reviews.apache.org/r/7544/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Reply via email to