Re: Review Request 37327: Added a recordio::Reader for wrapping an http::Pipe::Reader.

[Ben Mahler]

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

(Updated Aug. 11, 2015, 12:41 a.m.)


Review request for mesos, Anand Mazumdar, Benjamin Hindman, Jie Yu, and Vinod 
Kone.


Changes
---

Added documentation and improved one of the tests. No need for further reviews.


Repository: mesos


Description
---

This is helpful for the scheduler library and tests. Since we
currently do not have a generalized abstraction in libprocess
for "streams" of asynchronous data (e.g. process::Stream),
we have to create a one-off wrapper here.


Diffs (updated)
-

  src/Makefile.am 942003149b071a322933e2c085d9122903f69713 
  src/common/recordio.hpp PRE-CREATION 
  src/tests/common/recordio_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/37327/diff/


Testing
---

Added tests.


Thanks,

Ben Mahler

Re: Review Request 37327: Added a recordio::Reader for wrapping an http::Pipe::Reader.

[Ben Mahler]

> On Aug. 11, 2015, 12:13 a.m., Vinod Kone wrote:
> >

Added to the documentation that the caller is responsible for closing the pipe 
reader when a failure or end-of-file is encountered.


> On Aug. 11, 2015, 12:13 a.m., Vinod Kone wrote:
> > src/common/recordio.hpp, line 177
> > 
> >
> > do you need to pull this into its own function? it's only used one 
> > afaict.

I didn't need to, but it helps readability, no? I wanted it to be easy for 
someone to compare the failure and completion code.


> On Aug. 11, 2015, 12:13 a.m., Vinod Kone wrote:
> > src/common/recordio.hpp, line 207
> > 
> >
> > s/failure/error/ ? to be consistent with how we named this elsewhere in 
> > the codebase (e.g., group.cpp)

Ok, sounds good.


> On Aug. 11, 2015, 12:13 a.m., Vinod Kone wrote:
> > src/tests/common/recordio_tests.cpp, line 122
> > 
> >
> > can you also write a properly encoded record before and after you write 
> > the improper one?
> > 
> > i think that would be a more interesting test case.

Sounds good, thanks!


- Ben


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


On Aug. 10, 2015, 11:41 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37327/
> ---
> 
> (Updated Aug. 10, 2015, 11:41 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Hindman, Jie Yu, and Vinod 
> Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is helpful for the scheduler library and tests. Since we
> currently do not have a generalized abstraction in libprocess
> for "streams" of asynchronous data (e.g. process::Stream),
> we have to create a one-off wrapper here.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 942003149b071a322933e2c085d9122903f69713 
>   src/common/recordio.hpp PRE-CREATION 
>   src/tests/common/recordio_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37327/diff/
> 
> 
> Testing
> ---
> 
> Added tests.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 37327: Added a recordio::Reader for wrapping an http::Pipe::Reader.

[Vinod Kone]

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

Ship it!



src/common/recordio.hpp (line 177)


do you need to pull this into its own function? it's only used one afaict.



src/common/recordio.hpp (line 207)


s/failure/error/ ? to be consistent with how we named this elsewhere in the 
codebase (e.g., group.cpp)



src/tests/common/recordio_tests.cpp (line 122)


can you also write a properly encoded record before and after you write the 
improper one?

i think that would be a more interesting test case.


- Vinod Kone


On Aug. 10, 2015, 11:41 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37327/
> ---
> 
> (Updated Aug. 10, 2015, 11:41 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Hindman, Jie Yu, and Vinod 
> Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is helpful for the scheduler library and tests. Since we
> currently do not have a generalized abstraction in libprocess
> for "streams" of asynchronous data (e.g. process::Stream),
> we have to create a one-off wrapper here.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 942003149b071a322933e2c085d9122903f69713 
>   src/common/recordio.hpp PRE-CREATION 
>   src/tests/common/recordio_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37327/diff/
> 
> 
> Testing
> ---
> 
> Added tests.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Review Request 37327: Added a recordio::Reader for wrapping an http::Pipe::Reader.

[Ben Mahler]

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

Review request for mesos, Anand Mazumdar, Benjamin Hindman, Jie Yu, and Vinod 
Kone.


Repository: mesos


Description
---

This is helpful for the scheduler library and tests. Since we
currently do not have a generalized abstraction in libprocess
for "streams" of asynchronous data (e.g. process::Stream),
we have to create a one-off wrapper here.


Diffs
-

  src/Makefile.am 942003149b071a322933e2c085d9122903f69713 
  src/common/recordio.hpp PRE-CREATION 
  src/tests/common/recordio_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/37327/diff/


Testing
---

Added tests.


Thanks,

Ben Mahler