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

2015-08-10 Thread 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)
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::StreamT),
 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.

2015-08-10 Thread 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::StreamT),
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.

2015-08-10 Thread 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
  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::StreamT),
 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