You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Akash Gupta <ak...@hotmail.com> on 2018/05/30 18:56:54 UTC
Review Request 67386: Added `io::prepare_async` and `io::is_async`
functions for libprocess.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67386/
-----------------------------------------------------------
Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
Bugs: MESOS-5371 and MESOS-8668
https://issues.apache.org/jira/browse/MESOS-5371
https://issues.apache.org/jira/browse/MESOS-8668
Repository: mesos
Description
-------
`os::nonblock` and `os::isNonBlock` are stubs that only do things for
Windows sockets. For the IOCP implementation, we need to associate the
handles with the IOCP hande, which is created by libprocess. Thus, we
need these new functions to prepare the handle for the async IO
functions by either setting the hande to non-blocking on UNIX systems
or by associating it with an IOCP handle on Windows.
Diffs
-----
3rdparty/libprocess/include/process/io.hpp cc2caf44e065bed40263f3820e95a4f7c378bb98
3rdparty/libprocess/src/io.cpp 97f2b17092fbd23528cf3220fee5927a1ec38aba
3rdparty/libprocess/src/socket.cpp 504cb541785650d2d05aabd25f5258b9bad52baa
3rdparty/libprocess/src/tests/subprocess_tests.cpp 568d77b94b3afef8c1ebdde71e14aa0b498da5c8
Diff: https://reviews.apache.org/r/67386/diff/1/
Testing
-------
Thanks,
Akash Gupta
Re: Review Request 67386: Added `io::prepare_async` and `io::is_async`
functions for libprocess.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67386/#review205336
-----------------------------------------------------------
3rdparty/libprocess/include/process/io.hpp
Line 48 (original), 48-59 (patched)
<https://reviews.apache.org/r/67386/#comment288252>
Is there no ability to back to synchronous? I would expect something more like os::set_async / io::unset_async. Is there no way to cleanup the async set up without closing the fd?
This interface is also a little puzzling to me because it's relationship to os::nonblock is puzzling. Is stout's os::nonblock supposed to be cross-platform or POSIX only? Why is there an implementation of os::nonblock for windows?
https://github.com/apache/mesos/blob/1.6.0/3rdparty/stout/include/stout/os/windows/fcntl.hpp#L45-L65
Reading this code, is it the HANDLE case that's not supported but SOCKET is? If so, why does that return nothing rather than an error saying it's not supported? The code just saying "do nothing" is confusing to me.
For the SOCKET case, it seems to do something, does os::nonblock on a socket not enable me to use io::read (with IOCP)?
- Benjamin Mahler
On May 30, 2018, 6:56 p.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67386/
> -----------------------------------------------------------
>
> (Updated May 30, 2018, 6:56 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-5371 and MESOS-8668
> https://issues.apache.org/jira/browse/MESOS-5371
> https://issues.apache.org/jira/browse/MESOS-8668
>
>
> Repository: mesos
>
>
> Description
> -------
>
> `os::nonblock` and `os::isNonBlock` are stubs that only do things for
> Windows sockets. For the IOCP implementation, we need to associate the
> handles with the IOCP hande, which is created by libprocess. Thus, we
> need these new functions to prepare the handle for the async IO
> functions by either setting the hande to non-blocking on UNIX systems
> or by associating it with an IOCP handle on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/io.hpp cc2caf44e065bed40263f3820e95a4f7c378bb98
> 3rdparty/libprocess/src/io.cpp 97f2b17092fbd23528cf3220fee5927a1ec38aba
> 3rdparty/libprocess/src/socket.cpp 504cb541785650d2d05aabd25f5258b9bad52baa
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp 568d77b94b3afef8c1ebdde71e14aa0b498da5c8
>
>
> Diff: https://reviews.apache.org/r/67386/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Akash Gupta
>
>
Re: Review Request 67386: Added `io::prepare_async` and `io::is_async`
functions for libprocess.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67386/#review204498
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/libprocess/include/process/io.hpp
Lines 49 (patched)
<https://reviews.apache.org/r/67386/#comment286974>
s/UNIX/POSIX
3rdparty/libprocess/include/process/io.hpp
Lines 58-60 (patched)
<https://reviews.apache.org/r/67386/#comment286978>
Maybe expand that `asynchronous` on POSIX means non-blocking and on Windows means overlapped and associated with an IOCP.
3rdparty/libprocess/include/process/io.hpp
Lines 60-91 (original), 78-111 (patched)
<https://reviews.apache.org/r/67386/#comment286977>
Just to note: this has semantically been true all along, it's not a new requirement we're adding to libprocess.
3rdparty/libprocess/include/process/io.hpp
Line 88 (original), 107 (patched)
<https://reviews.apache.org/r/67386/#comment286975>
Missing a space before the first backtick
3rdparty/libprocess/src/io.cpp
Lines 136-147 (patched)
<https://reviews.apache.org/r/67386/#comment286976>
Just to clarify: the reason we're not using the `os::` functions directly is that for Windows IOCP this step requires access to information (the handle which is the IO Completion Port) only available at the level of libprocess, not stout, right? We should probably document this so this makes sense to non-Windows people.
- Andrew Schwartzmeyer
On May 30, 2018, 11:56 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67386/
> -----------------------------------------------------------
>
> (Updated May 30, 2018, 11:56 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-5371 and MESOS-8668
> https://issues.apache.org/jira/browse/MESOS-5371
> https://issues.apache.org/jira/browse/MESOS-8668
>
>
> Repository: mesos
>
>
> Description
> -------
>
> `os::nonblock` and `os::isNonBlock` are stubs that only do things for
> Windows sockets. For the IOCP implementation, we need to associate the
> handles with the IOCP hande, which is created by libprocess. Thus, we
> need these new functions to prepare the handle for the async IO
> functions by either setting the hande to non-blocking on UNIX systems
> or by associating it with an IOCP handle on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/io.hpp cc2caf44e065bed40263f3820e95a4f7c378bb98
> 3rdparty/libprocess/src/io.cpp 97f2b17092fbd23528cf3220fee5927a1ec38aba
> 3rdparty/libprocess/src/socket.cpp 504cb541785650d2d05aabd25f5258b9bad52baa
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp 568d77b94b3afef8c1ebdde71e14aa0b498da5c8
>
>
> Diff: https://reviews.apache.org/r/67386/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Akash Gupta
>
>
Re: Review Request 67386: Added `io::prepare_async` and `io::is_async`
functions for libprocess.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67386/#review205310
-----------------------------------------------------------
Ship it!
Ship It!
- Joseph Wu
On May 30, 2018, 11:56 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67386/
> -----------------------------------------------------------
>
> (Updated May 30, 2018, 11:56 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-5371 and MESOS-8668
> https://issues.apache.org/jira/browse/MESOS-5371
> https://issues.apache.org/jira/browse/MESOS-8668
>
>
> Repository: mesos
>
>
> Description
> -------
>
> `os::nonblock` and `os::isNonBlock` are stubs that only do things for
> Windows sockets. For the IOCP implementation, we need to associate the
> handles with the IOCP hande, which is created by libprocess. Thus, we
> need these new functions to prepare the handle for the async IO
> functions by either setting the hande to non-blocking on UNIX systems
> or by associating it with an IOCP handle on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/io.hpp cc2caf44e065bed40263f3820e95a4f7c378bb98
> 3rdparty/libprocess/src/io.cpp 97f2b17092fbd23528cf3220fee5927a1ec38aba
> 3rdparty/libprocess/src/socket.cpp 504cb541785650d2d05aabd25f5258b9bad52baa
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp 568d77b94b3afef8c1ebdde71e14aa0b498da5c8
>
>
> Diff: https://reviews.apache.org/r/67386/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Akash Gupta
>
>