Re: Review Request 67385: Windows: Added IOCP `HANDLE` to `WindowsFD`.

2018-06-25 Thread Joseph Wu 

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


Ship it!




Two more typos for Andy to fix before committing.


3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 203 (patched)


Nit: s/bookeeping/bookkeeping/



3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 215 (patched)


Nit: s/assigns/assign/


- Joseph Wu


On May 30, 2018, 11:58 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67385/
> ---
> 
> (Updated May 30, 2018, 11:58 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
> ---
> 
> Mesos/Libprocess uses `os::nonblock()` in a way that assumes that it is
> idempotent. Unfortunately, `CreateIoCompletionPort` is not idempotent,
> so we need to wrap the function around our own code to make it so. We
> need to keep it inside the `WindowsFD` class, because there isn't a way
> to determine if a HANDLE is associated with an IOCP through the Win32
> API.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/dup.hpp 
> 5bda095e676b038cdaea04f7be23ba2a1aca9015 
>   3rdparty/stout/include/stout/os/windows/fd.hpp 
> 89a037af31ad68a275d2519afbe4f161b23efe91 
> 
> 
> Diff: https://reviews.apache.org/r/67385/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 67385: Windows: Added IOCP `HANDLE` to `WindowsFD`.

2018-06-21 Thread Andrew Schwartzmeyer 

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


Ship it!





3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 245 (patched)


Nit that I'll fix when commiting: missing a space ;)


- Andrew Schwartzmeyer


On May 30, 2018, 11:58 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67385/
> ---
> 
> (Updated May 30, 2018, 11:58 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
> ---
> 
> Mesos/Libprocess uses `os::nonblock()` in a way that assumes that it is
> idempotent. Unfortunately, `CreateIoCompletionPort` is not idempotent,
> so we need to wrap the function around our own code to make it so. We
> need to keep it inside the `WindowsFD` class, because there isn't a way
> to determine if a HANDLE is associated with an IOCP through the Win32
> API.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/dup.hpp 
> 5bda095e676b038cdaea04f7be23ba2a1aca9015 
>   3rdparty/stout/include/stout/os/windows/fd.hpp 
> 89a037af31ad68a275d2519afbe4f161b23efe91 
> 
> 
> Diff: https://reviews.apache.org/r/67385/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 67385: Windows: Added IOCP `HANDLE` to `WindowsFD`.

2018-06-12 Thread Andrew Schwartzmeyer 


> On June 5, 2018, 2:02 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/os/windows/fd.hpp
> > Lines 183-184 (patched)
> > 
> >
> > Is there ever a case where `iocp_handle_` hasn't been allocated? Right 
> > now, this is implicitly relying on the fact that we chose to make the 
> > default constructor construct off the `HANDLE` ctor with 
> > `INVALID_HANDLE_VALUE`, and that ctor uses `make_shared`. Just wondering if 
> > we should refactor to make the allocation more explicit and certain (like 
> > maybe to `make_shared` in the default ctor, and ensure all ctors call it 
> > too, I don't know yet).
> 
> Akash Gupta wrote:
> THere isn't a case, although an allocated `iocp_handle_` is only needed 
> for the "valid" `HANDLE/SOCKET` constructors. I think it makes sense to have 
> the most general (most params) constructor call `make_shared` and the 
> narrower constructors call the more general one like how it works now.

That makes sense.


- Andrew


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


On May 30, 2018, 11:58 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67385/
> ---
> 
> (Updated May 30, 2018, 11:58 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
> ---
> 
> Mesos/Libprocess uses `os::nonblock()` in a way that assumes that it is
> idempotent. Unfortunately, `CreateIoCompletionPort` is not idempotent,
> so we need to wrap the function around our own code to make it so. We
> need to keep it inside the `WindowsFD` class, because there isn't a way
> to determine if a HANDLE is associated with an IOCP through the Win32
> API.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/dup.hpp 
> 5bda095e676b038cdaea04f7be23ba2a1aca9015 
>   3rdparty/stout/include/stout/os/windows/fd.hpp 
> 5dbdff2680370d123579c5e3fdd9b0e0adaf512e 
> 
> 
> Diff: https://reviews.apache.org/r/67385/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 67385: Windows: Added IOCP `HANDLE` to `WindowsFD`.

2018-06-05 Thread Andrew Schwartzmeyer 

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




3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 181 (patched)


Maybe just `HANDLE handle` ;)



3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 181-182 (patched)


What is the `key` used for? (Maybe we need a small excerpt from the MSDN 
docs for `CreateIoCompletionPort`.)



3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 183 (patched)


I'm not saying you should use it, or if you even want to use it, but there 
does exist some synchronization helpers in stout, under `synchronized.hpp`.



3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 183-184 (patched)


Is there ever a case where `iocp_handle_` hasn't been allocated? Right now, 
this is implicitly relying on the fact that we chose to make the default 
constructor construct off the `HANDLE` ctor with `INVALID_HANDLE_VALUE`, and 
that ctor uses `make_shared`. Just wondering if we should refactor to make the 
allocation more explicit and certain (like maybe to `make_shared` in the 
default ctor, and ensure all ctors call it too, I don't know yet).



3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 184 (patched)


`const`



3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 188 (patched)


Oi, I hate that our cast operators are implicit, `*this` threw me for a 
minute.



3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 202-215 (patched)


Old comment, but leaving for posterity:

I don't like this `WindowsFD::dup` method; it opens up a can of worms, such 
as: should we move the `os::dup` logic into this method (should we then start 
moving other `os::foo(int_fd)` logic into their own methods)? It also prompted 
me to think about this maybe being done in a copy ctor, but that doesn't make 
sense either.

At a minimum, let's make this `private` and make `os::dup` a `friend` so 
that `WindowsFD::dup` is clearly demarcated as an internal helper.

And once I wrote that, I almost want to suggest: make `os::dup` a friend, 
and just make a new `fd` and manually assign the `overlapped_` and 
`iocp_handle_` fields (that is, get rid of the helper abstraction). Not sure if 
that would be cleaner.



3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 231-235 (patched)


This is more like `struct IOCPData`, since it's both the handle and the 
mutex.



3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 237 (patched)


This could just be named `iocp_`



3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 239-257 (patched)


As discussed in-person, we can simplify all this logic (read: delete these 
ctors and the `dup` method) by making `os::dup` a friend, using the default 
copy ctor in `os::dup`, and then overwriting the `handle_` or `socket_` field 
with the duplicated object (which is then doable as a friend function).


- Andrew Schwartzmeyer


On May 30, 2018, 11:58 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67385/
> ---
> 
> (Updated May 30, 2018, 11:58 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
> ---
> 
> Mesos/Libprocess uses `os::nonblock()` in a way that assumes that it is
> idempotent. Unfortunately, `CreateIoCompletionPort` is not idempotent,
> so we need to wrap the function around our own code to make it so. We
> need to keep it inside the `WindowsFD` class, because there isn't a way
> to determine if a HANDLE is associated with an IOCP through the Win32
> API.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/dup.hpp 
> 5bda095e676b038cdaea04f7be23ba2a1aca9015 
>   3rdparty/stout/include/stout/os/windows/fd.hpp 
> 5dbdff2680370d123579c5e3fdd9b0e0adaf512e 
> 
> 
> Diff: https://reviews.apache.org/r/67385/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 67385: Windows: Added IOCP `HANDLE` to `WindowsFD`.

2018-06-05 Thread Joseph Wu 

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




3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 181 (patched)


Suggestion: It's easy to get `iocp_handle` and `iocp_handle_` mixed up 
while reading this function.  Consider renaming the parameter `iocp_handle` to 
`target`.


- Joseph Wu


On May 30, 2018, 11:58 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67385/
> ---
> 
> (Updated May 30, 2018, 11:58 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
> ---
> 
> Mesos/Libprocess uses `os::nonblock()` in a way that assumes that it is
> idempotent. Unfortunately, `CreateIoCompletionPort` is not idempotent,
> so we need to wrap the function around our own code to make it so. We
> need to keep it inside the `WindowsFD` class, because there isn't a way
> to determine if a HANDLE is associated with an IOCP through the Win32
> API.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/dup.hpp 
> 5bda095e676b038cdaea04f7be23ba2a1aca9015 
>   3rdparty/stout/include/stout/os/windows/fd.hpp 
> 5dbdff2680370d123579c5e3fdd9b0e0adaf512e 
> 
> 
> Diff: https://reviews.apache.org/r/67385/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Review Request 67385: Windows: Added IOCP `HANDLE` to `WindowsFD`.

2018-05-30 Thread Akash Gupta 

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

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
---

Mesos/Libprocess uses `os::nonblock()` in a way that assumes that it is
idempotent. Unfortunately, `CreateIoCompletionPort` is not idempotent,
so we need to wrap the function around our own code to make it so. We
need to keep it inside the `WindowsFD` class, because there isn't a way
to determine if a HANDLE is associated with an IOCP through the Win32
API.


Diffs
-

  3rdparty/stout/include/stout/os/windows/dup.hpp 
5bda095e676b038cdaea04f7be23ba2a1aca9015 
  3rdparty/stout/include/stout/os/windows/fd.hpp 
5dbdff2680370d123579c5e3fdd9b0e0adaf512e 


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


Testing
---


Thanks,

Akash Gupta