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:46:13 UTC

Review Request 67390: Windows: Integrated libwinio with libprocess code.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67390/
-----------------------------------------------------------

Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.


Bugs: MESOS-8668, MESOS-8671 and MESOS-8672
    https://issues.apache.org/jira/browse/MESOS-8668
    https://issues.apache.org/jira/browse/MESOS-8671
    https://issues.apache.org/jira/browse/MESOS-8672


Repository: mesos


Description
-------

The libprocess eventloops are used to implement the `EventLoop`,
SocketImpl` and `io::read/write` interfaces. Thus, by providing those
implementations using libwinio, libprocess now supports the libwinio
backend.


Diffs
-----

  3rdparty/libprocess/src/libwinio.hpp PRE-CREATION 
  3rdparty/libprocess/src/libwinio_eventloop.cpp PRE-CREATION 
  3rdparty/libprocess/src/libwinio_io.cpp PRE-CREATION 
  3rdparty/libprocess/src/libwinio_socket.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/67390/diff/1/


Testing
-------


Thanks,

Akash Gupta

Re: Review Request 67390: Windows: Integrated libwinio with libprocess code.

Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67390/#review205215
-----------------------------------------------------------


Ship it!




Ship It!

- Andrew Schwartzmeyer


On June 21, 2018, 4:56 a.m., Akash Gupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67390/
> -----------------------------------------------------------
> 
> (Updated June 21, 2018, 4:56 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8668, MESOS-8671 and MESOS-8672
>     https://issues.apache.org/jira/browse/MESOS-8668
>     https://issues.apache.org/jira/browse/MESOS-8671
>     https://issues.apache.org/jira/browse/MESOS-8672
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The libprocess eventloops are used to implement the `EventLoop`,
> SocketImpl` and `io::read/write` interfaces. Thus, by providing those
> implementations using libwinio, libprocess now supports the libwinio
> backend.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/windows/event_loop.hpp PRE-CREATION 
>   3rdparty/libprocess/src/windows/event_loop.cpp PRE-CREATION 
>   3rdparty/libprocess/src/windows/io.cpp PRE-CREATION 
>   3rdparty/libprocess/src/windows/poll_socket.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67390/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 67390: Windows: Integrated libwinio with libprocess code.

Posted by Benjamin Mahler <bm...@apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67390/#review205338
-----------------------------------------------------------




3rdparty/libprocess/src/windows/event_loop.hpp
Lines 16 (patched)
<https://reviews.apache.org/r/67390/#comment288254>

    What is this? Googling it doesn't yield any results, is it one of the headers owned by the mesos project? If so, why call it "windows/libwinio.hpp" instead of "windows/io.hpp"? The former makes it sounds like an external library?


- Benjamin Mahler


On June 21, 2018, 11:56 a.m., Akash Gupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67390/
> -----------------------------------------------------------
> 
> (Updated June 21, 2018, 11:56 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8668, MESOS-8671 and MESOS-8672
>     https://issues.apache.org/jira/browse/MESOS-8668
>     https://issues.apache.org/jira/browse/MESOS-8671
>     https://issues.apache.org/jira/browse/MESOS-8672
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The libprocess eventloops are used to implement the `EventLoop`,
> SocketImpl` and `io::read/write` interfaces. Thus, by providing those
> implementations using libwinio, libprocess now supports the libwinio
> backend.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/windows/event_loop.hpp PRE-CREATION 
>   3rdparty/libprocess/src/windows/event_loop.cpp PRE-CREATION 
>   3rdparty/libprocess/src/windows/io.cpp PRE-CREATION 
>   3rdparty/libprocess/src/windows/poll_socket.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67390/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 67390: Windows: Integrated libwinio with libprocess code.

Posted by Akash Gupta <ak...@hotmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67390/
-----------------------------------------------------------

(Updated June 21, 2018, 11:56 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.


Changes
-------

file rename and feedback.


Bugs: MESOS-8668, MESOS-8671 and MESOS-8672
    https://issues.apache.org/jira/browse/MESOS-8668
    https://issues.apache.org/jira/browse/MESOS-8671
    https://issues.apache.org/jira/browse/MESOS-8672


Repository: mesos


Description
-------

The libprocess eventloops are used to implement the `EventLoop`,
SocketImpl` and `io::read/write` interfaces. Thus, by providing those
implementations using libwinio, libprocess now supports the libwinio
backend.


Diffs (updated)
-----

  3rdparty/libprocess/src/windows/event_loop.hpp PRE-CREATION 
  3rdparty/libprocess/src/windows/event_loop.cpp PRE-CREATION 
  3rdparty/libprocess/src/windows/io.cpp PRE-CREATION 
  3rdparty/libprocess/src/windows/poll_socket.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/67390/diff/2/

Changes: https://reviews.apache.org/r/67390/diff/1-2/


Testing
-------


Thanks,

Akash Gupta

Re: Review Request 67390: Windows: Integrated libwinio with libprocess code.

Posted by Radhika Jandhyala via Review Board <no...@reviews.apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67390/#review204207
-----------------------------------------------------------




3rdparty/libprocess/src/libwinio_io.cpp
Lines 47 (patched)
<https://reviews.apache.org/r/67390/#comment286611>

    I am not sure what the rules are, but since this is retruning WindowsError and making calls to the windows namespace, should this be in stout?


- Radhika Jandhyala


On May 30, 2018, 6:46 p.m., Akash Gupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67390/
> -----------------------------------------------------------
> 
> (Updated May 30, 2018, 6:46 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8668, MESOS-8671 and MESOS-8672
>     https://issues.apache.org/jira/browse/MESOS-8668
>     https://issues.apache.org/jira/browse/MESOS-8671
>     https://issues.apache.org/jira/browse/MESOS-8672
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The libprocess eventloops are used to implement the `EventLoop`,
> SocketImpl` and `io::read/write` interfaces. Thus, by providing those
> implementations using libwinio, libprocess now supports the libwinio
> backend.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/libwinio.hpp PRE-CREATION 
>   3rdparty/libprocess/src/libwinio_eventloop.cpp PRE-CREATION 
>   3rdparty/libprocess/src/libwinio_io.cpp PRE-CREATION 
>   3rdparty/libprocess/src/libwinio_socket.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67390/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 67390: Windows: Integrated libwinio with libprocess code.

Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.


> On June 8, 2018, 3:46 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/libprocess/src/libwinio_io.cpp
> > Lines 78-101 (patched)
> > <https://reviews.apache.org/r/67390/diff/1/?file=2032286#file2032286line78>
> >
> >     Wait... doesn't this belong in the `prepare_async` from earlier? Did something get moved?
> 
> Akash Gupta wrote:
>     Hm what do you mean? There's two `prepare_asyncs`. One for POSIX (polling), one for Windows (IOCP). This is the first time for the Windows one.

Now I know that `libprocess/src/io.cpp` isn't POSIX only... and it has:

```
Try<Nothing> prepare_async(int_fd fd)
{
  // TODO(akagup): Add windows iocp.
  return os::nonblock(fd);
}


Try<bool> is_async(int_fd fd)
{
  // TODO(akagup): Add windows iocp.
  return os::isNonblock(fd);
}
```

added in https://reviews.apache.org/r/67386/


> On June 8, 2018, 3:46 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/libprocess/src/libwinio_socket.cpp
> > Lines 105 (patched)
> > <https://reviews.apache.org/r/67390/diff/1/?file=2032287#file2032287line105>
> >
> >     What is it, and why?
> 
> Akash Gupta wrote:
>     It bascially combines a lot of small packets into a single big packet, so it can improve throughout at the expense of latency. I only disable it because the POSIX `PollSocketImpl` disables it. I think the reason why it is disabled is because serving libprocess HTTP requets prefer latency over throughout.

Gotcha. Do they have anything more than this comment in their implementation? Maybe we could check the blame and find an issue...


- Andrew


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67390/#review204503
-----------------------------------------------------------


On May 30, 2018, 11:46 a.m., Akash Gupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67390/
> -----------------------------------------------------------
> 
> (Updated May 30, 2018, 11:46 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8668, MESOS-8671 and MESOS-8672
>     https://issues.apache.org/jira/browse/MESOS-8668
>     https://issues.apache.org/jira/browse/MESOS-8671
>     https://issues.apache.org/jira/browse/MESOS-8672
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The libprocess eventloops are used to implement the `EventLoop`,
> SocketImpl` and `io::read/write` interfaces. Thus, by providing those
> implementations using libwinio, libprocess now supports the libwinio
> backend.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/libwinio.hpp PRE-CREATION 
>   3rdparty/libprocess/src/libwinio_eventloop.cpp PRE-CREATION 
>   3rdparty/libprocess/src/libwinio_io.cpp PRE-CREATION 
>   3rdparty/libprocess/src/libwinio_socket.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67390/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 67390: Windows: Integrated libwinio with libprocess code.

Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67390/#review204503
-----------------------------------------------------------




3rdparty/libprocess/src/libwinio_eventloop.cpp
Lines 52-73 (patched)
<https://reviews.apache.org/r/67390/#comment287012>

    I know this code exists elsewhere too... we should refactor into a stout function.



3rdparty/libprocess/src/libwinio_io.cpp
Lines 78-101 (patched)
<https://reviews.apache.org/r/67390/#comment287013>

    Wait... doesn't this belong in the `prepare_async` from earlier? Did something get moved?



3rdparty/libprocess/src/libwinio_socket.cpp
Lines 16-18 (patched)
<https://reviews.apache.org/r/67390/#comment287014>

    Huh? Isn't this file called `libWINio_socket.cpp`?



3rdparty/libprocess/src/libwinio_socket.cpp
Lines 100-102 (patched)
<https://reviews.apache.org/r/67390/#comment287016>

    I think you want a `WindowsError error; ... + error.message`. Otherwise it's just going to print `socket: 3` or whatever.



3rdparty/libprocess/src/libwinio_socket.cpp
Lines 105 (patched)
<https://reviews.apache.org/r/67390/#comment287015>

    What is it, and why?



3rdparty/libprocess/src/libwinio_socket.cpp
Lines 147 (patched)
<https://reviews.apache.org/r/67390/#comment287017>

    What does that mean?



3rdparty/libprocess/src/libwinio_socket.cpp
Lines 176 (patched)
<https://reviews.apache.org/r/67390/#comment287018>

    This code looks very... reused. Nothing necessarily wrong with that, but should we reduce it at all?


- Andrew Schwartzmeyer


On May 30, 2018, 11:46 a.m., Akash Gupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67390/
> -----------------------------------------------------------
> 
> (Updated May 30, 2018, 11:46 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8668, MESOS-8671 and MESOS-8672
>     https://issues.apache.org/jira/browse/MESOS-8668
>     https://issues.apache.org/jira/browse/MESOS-8671
>     https://issues.apache.org/jira/browse/MESOS-8672
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The libprocess eventloops are used to implement the `EventLoop`,
> SocketImpl` and `io::read/write` interfaces. Thus, by providing those
> implementations using libwinio, libprocess now supports the libwinio
> backend.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/libwinio.hpp PRE-CREATION 
>   3rdparty/libprocess/src/libwinio_eventloop.cpp PRE-CREATION 
>   3rdparty/libprocess/src/libwinio_io.cpp PRE-CREATION 
>   3rdparty/libprocess/src/libwinio_socket.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67390/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>