You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2015/08/11 01:41:51 UTC
Review Request 37327: Added a recordio::Reader for wrapping an
http::Pipe::Reader.
-----------------------------------------------------------
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<T>),
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.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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<T>),
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.
Posted by Ben Mahler <be...@gmail.com>.
> 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
> > <https://reviews.apache.org/r/37327/diff/1/?file=1036864#file1036864line177>
> >
> > 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
> > <https://reviews.apache.org/r/37327/diff/1/?file=1036864#file1036864line207>
> >
> > 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
> > <https://reviews.apache.org/r/37327/diff/1/?file=1036865#file1036865line122>
> >
> > 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<T>),
> 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.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/37327/#comment149475>
do you need to pull this into its own function? it's only used one afaict.
src/common/recordio.hpp (line 207)
<https://reviews.apache.org/r/37327/#comment149472>
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)
<https://reviews.apache.org/r/37327/#comment149479>
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<T>),
> 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
>
>