You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2012/06/26 09:49:11 UTC
Review Request: Add an interface in process::io to do non-blocking read.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5576/
-----------------------------------------------------------
Review request for mesos and Benjamin Hindman.
Description
-------
Provide an interface in process::io to do non-blocking read. It leverage the io polling mechanism introduced in https://reviews.apache.org/r/5424/
Diffs
-----
third_party/libprocess/include/process/io.hpp 801a6d5
third_party/libprocess/src/process.cpp b7c181e
third_party/libprocess/src/tests.cpp 3d5005c
Diff: https://reviews.apache.org/r/5576/diff/
Testing
-------
cd /path/to/libprocess/build
./tests
Tested on both Mac and Linux.
Thanks,
Jie Yu
Re: Review Request: Add an interface in process::io to do non-blocking read.
Posted by Jie Yu <yu...@gmail.com>.
> On June 27, 2012, 4:29 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/include/process/io.hpp, line 33
> > <https://reviews.apache.org/r/5576/diff/2/?file=116485#file116485line33>
> >
> > The problem with waiting until 'size' bytes are actually read is that in certain circumstances that could take a very long time indeed (forever). Consider this scenario: the file descriptor is actually for a socket, the socket is waiting for a message, but the user doesn't know if the message will be 10 bytes, or 100 bytes, or 1000 bytes, etc. So it allocates a buffer of size 1000 bytes, and performs a read. If the message is only 10 bytes, it would like to get that data immediately. The decision about whether to read more (up until 'size') should really be pushed to the user. I'd be fine with another version of read which doesn't return until a certain number of bytes have been read, but that version should be implemented on top of this version.
Done. I keep both of them.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5576/#review8655
-----------------------------------------------------------
On June 26, 2012, 4:19 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5576/
> -----------------------------------------------------------
>
> (Updated June 26, 2012, 4:19 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> Provide an interface in process::io to do non-blocking read. It leverage the io polling mechanism introduced in https://reviews.apache.org/r/5424/
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/io.hpp 801a6d5
> third_party/libprocess/src/process.cpp b7c181e
> third_party/libprocess/src/tests.cpp 3d5005c
>
> Diff: https://reviews.apache.org/r/5576/diff/
>
>
> Testing
> -------
>
> cd /path/to/libprocess/build
> ./tests
>
> Tested on both Mac and Linux.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Add an interface in process::io to do non-blocking read.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5576/#review8655
-----------------------------------------------------------
third_party/libprocess/include/process/io.hpp
<https://reviews.apache.org/r/5576/#comment18316>
The problem with waiting until 'size' bytes are actually read is that in certain circumstances that could take a very long time indeed (forever). Consider this scenario: the file descriptor is actually for a socket, the socket is waiting for a message, but the user doesn't know if the message will be 10 bytes, or 100 bytes, or 1000 bytes, etc. So it allocates a buffer of size 1000 bytes, and performs a read. If the message is only 10 bytes, it would like to get that data immediately. The decision about whether to read more (up until 'size') should really be pushed to the user. I'd be fine with another version of read which doesn't return until a certain number of bytes have been read, but that version should be implemented on top of this version.
- Benjamin Hindman
On June 26, 2012, 4:19 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5576/
> -----------------------------------------------------------
>
> (Updated June 26, 2012, 4:19 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> Provide an interface in process::io to do non-blocking read. It leverage the io polling mechanism introduced in https://reviews.apache.org/r/5424/
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/io.hpp 801a6d5
> third_party/libprocess/src/process.cpp b7c181e
> third_party/libprocess/src/tests.cpp 3d5005c
>
> Diff: https://reviews.apache.org/r/5576/diff/
>
>
> Testing
> -------
>
> cd /path/to/libprocess/build
> ./tests
>
> Tested on both Mac and Linux.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Add an interface in process::io to do non-blocking read.
Posted by Jie Yu <yu...@gmail.com>.
> On June 27, 2012, 8:15 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/include/process/io.hpp, line 44
> > <https://reviews.apache.org/r/5576/diff/3/?file=116935#file116935line44>
> >
> > Let's simplify this more: readAll either returns all the data, or an error (no need for 'actual'). Anyone who wants to get "partial" data will just use 'read'.
Remove readAll as no efficient way to provide transactional semantics.
> On June 27, 2012, 8:15 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/process.cpp, line 3005
> > <https://reviews.apache.org/r/5576/diff/3/?file=116936#file116936line3005>
> >
> > Kill space.
Done.
> On June 27, 2012, 8:15 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/process.cpp, line 3023
> > <https://reviews.apache.org/r/5576/diff/3/?file=116936#file116936line3023>
> >
> > I'd love to move this out as a CHECK before line 3017, since these semantics should be guaranteed by our implementation. Move the comment too of course. ;)
Done.
> On June 27, 2012, 8:15 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/process.cpp, line 3151
> > <https://reviews.apache.org/r/5576/diff/3/?file=116936#file116936line3151>
> >
> > Let's implement 'readAll' interms of 'read'.
Removed.
> On June 27, 2012, 8:15 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/tests.cpp, lines 1113-1114
> > <https://reviews.apache.org/r/5576/diff/3/?file=116937#file116937line1113>
> >
> > For tests, you can just stick stuff like this on one line (see previous tests):
> >
> > ASSERT_NE(-1, pipe(pipes));
Done.
> On June 27, 2012, 8:15 p.m., Benjamin Hindman wrote:
> > third_party/libprocess/src/tests.cpp, line 1133
> > <https://reviews.apache.org/r/5576/diff/3/?file=116937#file116937line1133>
> >
> > Let's factor out making a file descriptor blocking into the process::io namespace. This will be incredibly useful beyond just testing!
> >
> > namespace io {
> >
> > bool nonblock(int fd)
> > {
> > ... fcntl(...);
> > }
> >
> > }
> >
> > It would be great to have a way to check if something is non-blocking too (maybe nonblock(fd) and isNonblock(fd)).
> >
Done.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5576/#review8664
-----------------------------------------------------------
On June 27, 2012, 6:44 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5576/
> -----------------------------------------------------------
>
> (Updated June 27, 2012, 6:44 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> Provide an interface in process::io to do non-blocking read. It leverage the io polling mechanism introduced in https://reviews.apache.org/r/5424/
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/io.hpp 801a6d5
> third_party/libprocess/src/process.cpp b7c181e
> third_party/libprocess/src/tests.cpp 3d5005c
>
> Diff: https://reviews.apache.org/r/5576/diff/
>
>
> Testing
> -------
>
> cd /path/to/libprocess/build
> ./tests
>
> Tested on both Mac and Linux.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Add an interface in process::io to do non-blocking read.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5576/#review8664
-----------------------------------------------------------
third_party/libprocess/include/process/io.hpp
<https://reviews.apache.org/r/5576/#comment18324>
Let's simplify this more: readAll either returns all the data, or an error (no need for 'actual'). Anyone who wants to get "partial" data will just use 'read'.
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/5576/#comment18318>
Kill space.
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/5576/#comment18325>
I'd love to move this out as a CHECK before line 3017, since these semantics should be guaranteed by our implementation. Move the comment too of course. ;)
third_party/libprocess/src/process.cpp
<https://reviews.apache.org/r/5576/#comment18326>
Let's implement 'readAll' interms of 'read'.
third_party/libprocess/src/tests.cpp
<https://reviews.apache.org/r/5576/#comment18322>
For tests, you can just stick stuff like this on one line (see previous tests):
ASSERT_NE(-1, pipe(pipes));
third_party/libprocess/src/tests.cpp
<https://reviews.apache.org/r/5576/#comment18323>
Let's factor out making a file descriptor blocking into the process::io namespace. This will be incredibly useful beyond just testing!
namespace io {
bool nonblock(int fd)
{
... fcntl(...);
}
}
It would be great to have a way to check if something is non-blocking too (maybe nonblock(fd) and isNonblock(fd)).
- Benjamin Hindman
On June 27, 2012, 6:44 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5576/
> -----------------------------------------------------------
>
> (Updated June 27, 2012, 6:44 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> Provide an interface in process::io to do non-blocking read. It leverage the io polling mechanism introduced in https://reviews.apache.org/r/5424/
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/io.hpp 801a6d5
> third_party/libprocess/src/process.cpp b7c181e
> third_party/libprocess/src/tests.cpp 3d5005c
>
> Diff: https://reviews.apache.org/r/5576/diff/
>
>
> Testing
> -------
>
> cd /path/to/libprocess/build
> ./tests
>
> Tested on both Mac and Linux.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Add an interface in process::io to do non-blocking read.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5576/
-----------------------------------------------------------
(Updated July 30, 2012, 11:13 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Split it into two patches so that it does not conflict with:
https://reviews.apache.org/r/6146/
This patch depends on:
https://reviews.apache.org/r/6217/
Description
-------
Provide an interface in process::io to do non-blocking read. It leverage the io polling mechanism introduced in https://reviews.apache.org/r/5424/
Diffs (updated)
-----
third_party/libprocess/include/process/io.hpp 801a6d5
third_party/libprocess/src/process.cpp 5c657d5
third_party/libprocess/src/tests.cpp 6d5d246
Diff: https://reviews.apache.org/r/5576/diff/
Testing
-------
cd /path/to/libprocess/build
./tests
Tested on both Mac and Linux.
Thanks,
Jie Yu
Re: Review Request: Add an interface in process::io to do non-blocking read.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5576/
-----------------------------------------------------------
(Updated July 16, 2012, 6 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Updated to fit the new trunk.
Description
-------
Provide an interface in process::io to do non-blocking read. It leverage the io polling mechanism introduced in https://reviews.apache.org/r/5424/
Diffs (updated)
-----
third_party/libprocess/include/process/io.hpp 801a6d5
third_party/libprocess/src/process.cpp b560b7c
third_party/libprocess/src/tests.cpp 6d5d246
Diff: https://reviews.apache.org/r/5576/diff/
Testing
-------
cd /path/to/libprocess/build
./tests
Tested on both Mac and Linux.
Thanks,
Jie Yu
Re: Review Request: Add an interface in process::io to do non-blocking read.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5576/#review8677
-----------------------------------------------------------
Ship it!
Awesome.
- Benjamin Hindman
On June 27, 2012, 11 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5576/
> -----------------------------------------------------------
>
> (Updated June 27, 2012, 11 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> Provide an interface in process::io to do non-blocking read. It leverage the io polling mechanism introduced in https://reviews.apache.org/r/5424/
>
>
> Diffs
> -----
>
> third_party/libprocess/include/process/io.hpp 801a6d5
> third_party/libprocess/src/process.cpp b7c181e
> third_party/libprocess/src/tests.cpp 3d5005c
>
> Diff: https://reviews.apache.org/r/5576/diff/
>
>
> Testing
> -------
>
> cd /path/to/libprocess/build
> ./tests
>
> Tested on both Mac and Linux.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Add an interface in process::io to do non-blocking read.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5576/
-----------------------------------------------------------
(Updated June 27, 2012, 11 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Address Ben's feedback.
Description
-------
Provide an interface in process::io to do non-blocking read. It leverage the io polling mechanism introduced in https://reviews.apache.org/r/5424/
Diffs (updated)
-----
third_party/libprocess/include/process/io.hpp 801a6d5
third_party/libprocess/src/process.cpp b7c181e
third_party/libprocess/src/tests.cpp 3d5005c
Diff: https://reviews.apache.org/r/5576/diff/
Testing
-------
cd /path/to/libprocess/build
./tests
Tested on both Mac and Linux.
Thanks,
Jie Yu
Re: Review Request: Add an interface in process::io to do non-blocking read.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5576/
-----------------------------------------------------------
(Updated June 27, 2012, 6:44 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Address Ben's feedback.
Description
-------
Provide an interface in process::io to do non-blocking read. It leverage the io polling mechanism introduced in https://reviews.apache.org/r/5424/
Diffs (updated)
-----
third_party/libprocess/include/process/io.hpp 801a6d5
third_party/libprocess/src/process.cpp b7c181e
third_party/libprocess/src/tests.cpp 3d5005c
Diff: https://reviews.apache.org/r/5576/diff/
Testing
-------
cd /path/to/libprocess/build
./tests
Tested on both Mac and Linux.
Thanks,
Jie Yu
Re: Review Request: Add an interface in process::io to do non-blocking read.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5576/
-----------------------------------------------------------
(Updated June 26, 2012, 4:19 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Fix format issues. Nicer comments.
Description
-------
Provide an interface in process::io to do non-blocking read. It leverage the io polling mechanism introduced in https://reviews.apache.org/r/5424/
Diffs (updated)
-----
third_party/libprocess/include/process/io.hpp 801a6d5
third_party/libprocess/src/process.cpp b7c181e
third_party/libprocess/src/tests.cpp 3d5005c
Diff: https://reviews.apache.org/r/5576/diff/
Testing
-------
cd /path/to/libprocess/build
./tests
Tested on both Mac and Linux.
Thanks,
Jie Yu