You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2018/06/04 23:53:57 UTC
Re: Review Request 67388: Moved: `io::internal::read/write` to
separate file.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67388/#review204289
-----------------------------------------------------------
General note on code organization:
Since you are splitting the `io::internal::read/write` functions into Posix and Windows implementations, it would be helpful to organize the code like:
```
3rdparty/libprocess/src/
|-- io/
|-- io.cpp
|
|-- posix/
| |-- io.hpp // Instead of io_internal.hpp
| |-- io.cpp // Instead of poll_io.cpp
|
|-- windows/
| // Instead of libwinio_impl.hpp ( https://reviews.apache.org/r/67389/ )
| // Possibly combine libwinio.hpp too ( https://reviews.apache.org/r/67390/ )
|-- event_loop.hpp
|
| // Instead of libwinio_impl.cpp ( https://reviews.apache.org/r/67389/ )
| // Possibly combine libwinio_eventloop.cpp too ( https://reviews.apache.org/r/67390/ )
|-- event_loop.cpp
|
|-- io.hpp
|-- io.cpp // Instead of libwinio_io.cpp ( https://reviews.apache.org/r/67390/ )
|-- ...
```
- Joseph Wu
On May 30, 2018, 11:55 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67388/
> -----------------------------------------------------------
>
> (Updated May 30, 2018, 11:55 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-8668
> https://issues.apache.org/jira/browse/MESOS-8668
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The implementation in `io::internal::read/write` uses `io::poll`, which
> is UNIX specific. The Windows IOCP implementation will not use a
> polling function, since there no unified mechanism, so the functions
> have been moved to their own file.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am d434001fbc49d337b6e29f6ac8c9c7475922a819
> 3rdparty/libprocess/src/CMakeLists.txt cf443dffd0663ecf02b7efd6f7094175b94aae19
> 3rdparty/libprocess/src/io.cpp 97f2b17092fbd23528cf3220fee5927a1ec38aba
> 3rdparty/libprocess/src/io_internal.hpp PRE-CREATION
> 3rdparty/libprocess/src/poll_io.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67388/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Akash Gupta
>
>
Re: Review Request 67388: Moved: `io::internal::read/write` to
separate file.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On June 4, 2018, 4:53 p.m., Joseph Wu wrote:
> > General note on code organization:
> >
> > Since you are splitting the `io::internal::read/write` functions into Posix and Windows implementations, it would be helpful to organize the code like:
> > ```
> > 3rdparty/libprocess/src/
> > |-- io/
> > |-- io.cpp
> > |
> > |-- posix/
> > | |-- io.hpp // Instead of io_internal.hpp
> > | |-- io.cpp // Instead of poll_io.cpp
> > |
> > |-- windows/
> > | // Instead of libwinio_impl.hpp ( https://reviews.apache.org/r/67389/ )
> > | // Possibly combine libwinio.hpp too ( https://reviews.apache.org/r/67390/ )
> > |-- event_loop.hpp
> > |
> > | // Instead of libwinio_impl.cpp ( https://reviews.apache.org/r/67389/ )
> > | // Possibly combine libwinio_eventloop.cpp too ( https://reviews.apache.org/r/67390/ )
> > |-- event_loop.cpp
> > |
> > |-- io.hpp
> > |-- io.cpp // Instead of libwinio_io.cpp ( https://reviews.apache.org/r/67390/ )
> > |-- ...
> > ```
+1
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67388/#review204289
-----------------------------------------------------------
On May 30, 2018, 11:55 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67388/
> -----------------------------------------------------------
>
> (Updated May 30, 2018, 11:55 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-8668
> https://issues.apache.org/jira/browse/MESOS-8668
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The implementation in `io::internal::read/write` uses `io::poll`, which
> is UNIX specific. The Windows IOCP implementation will not use a
> polling function, since there no unified mechanism, so the functions
> have been moved to their own file.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am d434001fbc49d337b6e29f6ac8c9c7475922a819
> 3rdparty/libprocess/src/CMakeLists.txt cf443dffd0663ecf02b7efd6f7094175b94aae19
> 3rdparty/libprocess/src/io.cpp 97f2b17092fbd23528cf3220fee5927a1ec38aba
> 3rdparty/libprocess/src/io_internal.hpp PRE-CREATION
> 3rdparty/libprocess/src/poll_io.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67388/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Akash Gupta
>
>
Re: Review Request 67388: Moved libprocess POSIX and Windows
implementations to separate folders.
Posted by Benjamin Mahler <bm...@apache.org>.
> On June 4, 2018, 11:53 p.m., Joseph Wu wrote:
> > General note on code organization:
> >
> > Since you are splitting the `io::internal::read/write` functions into Posix and Windows implementations, it would be helpful to organize the code like:
> > ```
> > 3rdparty/libprocess/src/
> > |-- io/
> > |-- io.cpp
> > |
> > |-- posix/
> > | |-- io.hpp // Instead of io_internal.hpp
> > | |-- io.cpp // Instead of poll_io.cpp
> > |
> > |-- windows/
> > | // Instead of libwinio_impl.hpp ( https://reviews.apache.org/r/67389/ )
> > | // Possibly combine libwinio.hpp too ( https://reviews.apache.org/r/67390/ )
> > |-- event_loop.hpp
> > |
> > | // Instead of libwinio_impl.cpp ( https://reviews.apache.org/r/67389/ )
> > | // Possibly combine libwinio_eventloop.cpp too ( https://reviews.apache.org/r/67390/ )
> > |-- event_loop.cpp
> > |
> > |-- io.hpp
> > |-- io.cpp // Instead of libwinio_io.cpp ( https://reviews.apache.org/r/67390/ )
> > |-- ...
> > ```
>
> Andrew Schwartzmeyer wrote:
> +1
Doesn't this type of approach mean that I have look at two different headers (POSIX and Windows) whenever I want to write cross-platform code? I would hope that I only have to look at one header unless I need to #ifdef in the call site.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67388/#review204289
-----------------------------------------------------------
On June 21, 2018, 11:47 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67388/
> -----------------------------------------------------------
>
> (Updated June 21, 2018, 11:47 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-8668
> https://issues.apache.org/jira/browse/MESOS-8668
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In preparation of the new Windows IOCP library, the POSIX and Windows
> specific files in libprocess have been moved to their own directories
> for better code organization.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am 8910416ce4313a0d70721cf1bb1d1453aaf691f9
> 3rdparty/libprocess/src/CMakeLists.txt 619183eff6d6d301a011ab03f007410f50a0aa4f
> 3rdparty/libprocess/src/io.cpp 97f2b17092fbd23528cf3220fee5927a1ec38aba
> 3rdparty/libprocess/src/io_internal.hpp PRE-CREATION
> 3rdparty/libprocess/src/libev.hpp
> 3rdparty/libprocess/src/libev.cpp
> 3rdparty/libprocess/src/libev_poll.cpp
> 3rdparty/libprocess/src/libevent.hpp
> 3rdparty/libprocess/src/libevent.cpp
> 3rdparty/libprocess/src/libevent_poll.cpp
> 3rdparty/libprocess/src/libevent_ssl_socket.hpp
> 3rdparty/libprocess/src/libevent_ssl_socket.cpp
> 3rdparty/libprocess/src/poll_socket.cpp
> 3rdparty/libprocess/src/posix/io.cpp PRE-CREATION
> 3rdparty/libprocess/src/socket.cpp 504cb541785650d2d05aabd25f5258b9bad52baa
> 3rdparty/libprocess/src/subprocess.cpp 0b2c02a9651563961532fdd5ab0f6d558f69f74e
> 3rdparty/libprocess/src/subprocess_posix.hpp
> 3rdparty/libprocess/src/subprocess_posix.cpp
> 3rdparty/libprocess/src/subprocess_windows.hpp
> 3rdparty/libprocess/src/subprocess_windows.cpp
>
>
> Diff: https://reviews.apache.org/r/67388/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Akash Gupta
>
>