You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alex Clemmer <cl...@gmail.com> on 2016/04/11 11:02:41 UTC
Review Request 46013: Stout: Implemented `os::processes` on Windows.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
-----------------------------------------------------------
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471
Repository: mesos
Description
-------
Stout: Implemented `os::processes` on Windows.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
Diff: https://reviews.apache.org/r/46013/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Alex Clemmer <cl...@gmail.com>.
> On April 12, 2016, 11:10 p.m., Alex Naparu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp, line 383
> > <https://reviews.apache.org/r/46013/diff/2/?file=1339790#file1339790line383>
> >
> > Can you document this flag a bit please? GetProcessTimes requires PROCESS_QUERY_LIMITED_INFORMATION, while GetProcessMemoryInfo requires the additional PROCESS_VM_READ.
Sure, but what part would you like me to document?
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review128565
-----------------------------------------------------------
On April 13, 2016, 9:33 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> -----------------------------------------------------------
>
> (Updated April 13, 2016, 9:33 p.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Implemented `os::processes` on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/46013/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Alex Clemmer <cl...@gmail.com>.
> On April 12, 2016, 11:10 p.m., Alex Naparu wrote:
> >
Hey, sorry, you reviewed an old version of the code. I had a different branch checked out when I pushed this. The background is that this is an amalgamation of maybe 3 or 4 commits, and the code is not amazing. I went through and fixed up a bunch of stuff on behalf of others, since they don't have the cycles to deal with it themselves.
Anyway, all this is to say: I've pushed a new revision, so most of these comments will be addressed incidentally -- things like `exists` being a parameter, stout naming conventions, etc.
Sorry for the confusion.
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review128565
-----------------------------------------------------------
On April 13, 2016, 9:33 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> -----------------------------------------------------------
>
> (Updated April 13, 2016, 9:33 p.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Implemented `os::processes` on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/46013/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Alex Naparu <al...@outlook.com>.
> On April 12, 2016, 11:10 p.m., Alex Naparu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp, line 383
> > <https://reviews.apache.org/r/46013/diff/2/?file=1339790#file1339790line383>
> >
> > Can you document this flag a bit please? GetProcessTimes requires PROCESS_QUERY_LIMITED_INFORMATION, while GetProcessMemoryInfo requires the additional PROCESS_VM_READ.
>
> Alex Clemmer wrote:
> Sure, but what part would you like me to document?
I would like to see some explanatory note as to why the THREAD_ALL_ACCESS flag is required (and appropriate) in this context. Looking at MSDN, none of the functions that are using the process handle (GetProcessTimes and GetProcessMemoryInfo) make any note of this flag. It might be that THREAD_ALL_ACCESS is overkill in this context.
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review128565
-----------------------------------------------------------
On April 13, 2016, 9:49 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> -----------------------------------------------------------
>
> (Updated April 13, 2016, 9:49 p.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Implemented `os::processes` on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/46013/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Alex Naparu <al...@outlook.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review128565
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 164)
<https://reviews.apache.org/r/46013/#comment191997>
I assume "disappear" means "between enumeration and now". Can you expand this comment a bit please?
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 270)
<https://reviews.apache.org/r/46013/#comment192018>
Doesn't follow stout naming convention (and can be mistaken for a WinAPI function).
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 272)
<https://reviews.apache.org/r/46013/#comment192025>
Not entirely sure we need this output arg
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 286)
<https://reviews.apache.org/r/46013/#comment192020>
spacing
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 289)
<https://reviews.apache.org/r/46013/#comment192023>
Comparing a HANDLE to NULL is a risky thing to do. I don't think MSDN defines NULL (which is defined as 0 in WinAPI) as an invalid handle value. We might just trigger a false negative here. Any HANDLE result should only be tested against INVALID_HANDLE_VALUE.
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 294)
<https://reviews.apache.org/r/46013/#comment192024>
I think we have some sort of safe_handle type by now, right?
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 304)
<https://reviews.apache.org/r/46013/#comment192027>
This can probably be scoped to each code block. No need to define it here, just creates confusion.
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 339)
<https://reviews.apache.org/r/46013/#comment192028>
These can all be on the same line.
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 372)
<https://reviews.apache.org/r/46013/#comment192015>
I don't think we should be using this flag. The fact that FindProcess returned an error is a solid indication that something is wrong.
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 382)
<https://reviews.apache.org/r/46013/#comment192019>
Can you qualify these calls with `::` please?
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 383)
<https://reviews.apache.org/r/46013/#comment192009>
Can you document this flag a bit please? GetProcessTimes requires PROCESS_QUERY_LIMITED_INFORMATION, while GetProcessMemoryInfo requires the additional PROCESS_VM_READ.
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 399)
<https://reviews.apache.org/r/46013/#comment192011>
Extra newline?
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 419)
<https://reviews.apache.org/r/46013/#comment192012>
Newline
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 423)
<https://reviews.apache.org/r/46013/#comment192007>
I don't think these can be negative, consider using ULARGE_INTEGER instead.
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 429)
<https://reviews.apache.org/r/46013/#comment192008>
Do you need system_time and user_time? I think these can be inlined below.
- Alex Naparu
On April 11, 2016, 7:19 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> -----------------------------------------------------------
>
> (Updated April 11, 2016, 7:19 p.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Implemented `os::processes` on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/46013/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Alex Naparu <al...@outlook.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review128781
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 310)
<https://reviews.apache.org/r/46013/#comment192236>
`break` is not needed after `return`
- Alex Naparu
On April 13, 2016, 10:06 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> -----------------------------------------------------------
>
> (Updated April 13, 2016, 10:06 p.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Implemented `os::processes` on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/46013/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Alex Clemmer <cl...@gmail.com>.
> On April 14, 2016, 9:10 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, lines 173-183
> > <https://reviews.apache.org/r/46013/diff/6/?file=1344018#file1344018line173>
> >
> > (1) It seems like this should live in `posix/os.hpp`.
> > (2) The declaration of this function in `windows/os.hpp` is:
> > ```
> > inline Result<Process> process(pid_t pid);
> > ```
> > It seems like the windows version needs all 3 states whereas the POSIX version only needs 2?
> > If that is the case, I think it makes sense to consolidate to using `Result` in both cases and
> > leave a comment on the POSIX version that we have the 3 states because of Windows.
I think I must be missing something. There is a version of `process` that returns a `Result` in `linux.hpp`, `osx.hpp`, and so on -- the version whose prototype you mention here is just the Windows implementation of this.
For the function you are highlighting here in your comment, which returns `Option`, this is separate -- it is meant to be shared among all platforms. There is no need for it to return `Result` because it is just finding a process with a certain `pid` in a list of processes.
So, my recommendation is to keep it the way it is.
Thoughts?
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review128983
-----------------------------------------------------------
On April 15, 2016, 7:50 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> -----------------------------------------------------------
>
> (Updated April 15, 2016, 7:50 a.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Implemented `os::processes` on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/46013/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Michael Park <mp...@apache.org>.
> On April 14, 2016, 9:10 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, lines 173-183
> > <https://reviews.apache.org/r/46013/diff/6/?file=1344018#file1344018line173>
> >
> > (1) It seems like this should live in `posix/os.hpp`.
> > (2) The declaration of this function in `windows/os.hpp` is:
> > ```
> > inline Result<Process> process(pid_t pid);
> > ```
> > It seems like the windows version needs all 3 states whereas the POSIX version only needs 2?
> > If that is the case, I think it makes sense to consolidate to using `Result` in both cases and
> > leave a comment on the POSIX version that we have the 3 states because of Windows.
>
> Alex Clemmer wrote:
> I think I must be missing something. There is a version of `process` that returns a `Result` in `linux.hpp`, `osx.hpp`, and so on -- the version whose prototype you mention here is just the Windows implementation of this.
>
> For the function you are highlighting here in your comment, which returns `Option`, this is separate -- it is meant to be shared among all platforms. There is no need for it to return `Result` because it is just finding a process with a certain `pid` in a list of processes.
>
> So, my recommendation is to keep it the way it is.
>
> Thoughts?
Yep, you're right. I read incorrectly. Too bad that we have `Option<Process> process(pid_t, const std::list<Process>&);` as well as `Result<Process> process(pid_t);` but that's a separate issue :)
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review128983
-----------------------------------------------------------
On April 15, 2016, 7:50 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> -----------------------------------------------------------
>
> (Updated April 15, 2016, 7:50 a.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Implemented `os::processes` on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/46013/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review128983
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 155)
<https://reviews.apache.org/r/46013/#comment192436>
Remove newline.
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 161)
<https://reviews.apache.org/r/46013/#comment192435>
`s/foreach(/foreach (/`
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (lines 173 - 183)
<https://reviews.apache.org/r/46013/#comment192434>
(1) It seems like this should live in `posix/os.hpp`.
(2) The declaration of this function in `windows/os.hpp` is:
```
inline Result<Process> process(pid_t pid);
```
It seems like the windows version needs all 3 states whereas the POSIX version only needs 2?
If that is the case, I think it makes sense to consolidate to using `Result` in both cases and
leave a comment on the POSIX version that we have the 3 states because of Windows.
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 200)
<https://reviews.apache.org/r/46013/#comment192439>
`s/foreach(/foreach (/`
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 202)
<https://reviews.apache.org/r/46013/#comment192440>
Indent 2 more spaces.
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 214)
<https://reviews.apache.org/r/46013/#comment192441>
`s/> >/>>/`
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 217)
<https://reviews.apache.org/r/46013/#comment192442>
Remove newline.
- Michael Park
On April 14, 2016, 8:57 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> -----------------------------------------------------------
>
> (Updated April 14, 2016, 8:57 a.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Implemented `os::processes` on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/46013/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review129192
-----------------------------------------------------------
Ship it!
Ship It!
- Michael Park
On April 15, 2016, 7:50 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> -----------------------------------------------------------
>
> (Updated April 15, 2016, 7:50 a.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Implemented `os::processes` on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/46013/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Alex Clemmer <cl...@gmail.com>.
> On April 15, 2016, 11:31 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp, line 362
> > <https://reviews.apache.org/r/46013/diff/7/?file=1346286#file1346286line362>
> >
> > What's the difference between `Failed to call X` vs `Call to `X` failed`?
Ah, I tried to be consistent about making all these messages say `Call to x failed` because I thought it was clearer. That is why there are both -- I just didn't get all of them.
Also, this message is somewhat inconsistent with our style. We say `os::processes: Call to x failed`. So we should fix that here too.
> On April 15, 2016, 11:31 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp, lines 406-407
> > <https://reviews.apache.org/r/46013/diff/7/?file=1346286#file1346286line406>
> >
> > I think it would easier to read:
> >
> > ```
> > Try<Duration> utime = Nanoseconds(lKernelTime.QuadPart * 100);
> > Try<Duration> stime = Nanoseconds(lUserTime.QuadPart * 100);
> > ```
> >
> > This is also arguably more correct, since any number < 10,000,000 would incorrectly result in 0 by the division.
> >
> > What do you think?
You're right, this was sloppy.
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review129204
-----------------------------------------------------------
On April 15, 2016, 7:50 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> -----------------------------------------------------------
>
> (Updated April 15, 2016, 7:50 a.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Implemented `os::processes` on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/46013/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review129204
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 294)
<https://reviews.apache.org/r/46013/#comment192660>
Should we use `SharedHandle` here?
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (lines 312 - 313)
<https://reviews.apache.org/r/46013/#comment192661>
`} else {`
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 359)
<https://reviews.apache.org/r/46013/#comment192663>
What's the difference between `Failed to call X` vs `Call to `X` failed`?
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (lines 403 - 404)
<https://reviews.apache.org/r/46013/#comment192667>
I think it would easier to read:
```
Try<Duration> utime = Nanoseconds(lKernelTime.QuadPart * 100);
Try<Duration> stime = Nanoseconds(lUserTime.QuadPart * 100);
```
This is also arguably more correct, since any number < 10,000,000 would incorrectly result in 0 by the division.
What do you think?
- Michael Park
On April 15, 2016, 7:50 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> -----------------------------------------------------------
>
> (Updated April 15, 2016, 7:50 a.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Implemented `os::processes` on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/46013/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/#review129642
-----------------------------------------------------------
Ship it!
Ship It!
- Michael Park
On April 18, 2016, 7:09 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> -----------------------------------------------------------
>
> (Updated April 18, 2016, 7:09 p.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Implemented `os::processes` on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/46013/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
-----------------------------------------------------------
(Updated April 21, 2016, 4:07 p.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471
Repository: mesos
Description
-------
Stout: Implemented `os::processes` on Windows.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp a81086860a17bd1912e55051dd7bb98a20772b51
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp f1584d0786ee0236a0e703d4aa9532e138ad49f0
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 37ef33257830e32875500537df7af38757c6efac
Diff: https://reviews.apache.org/r/46013/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
-----------------------------------------------------------
(Updated April 18, 2016, 7:09 p.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471
Repository: mesos
Description
-------
Stout: Implemented `os::processes` on Windows.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
Diff: https://reviews.apache.org/r/46013/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
-----------------------------------------------------------
(Updated April 17, 2016, 10:18 p.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471
Repository: mesos
Description
-------
Stout: Implemented `os::processes` on Windows.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
Diff: https://reviews.apache.org/r/46013/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
-----------------------------------------------------------
(Updated April 15, 2016, 7:50 a.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471
Repository: mesos
Description
-------
Stout: Implemented `os::processes` on Windows.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
Diff: https://reviews.apache.org/r/46013/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
-----------------------------------------------------------
(Updated April 14, 2016, 8:57 a.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471
Repository: mesos
Description
-------
Stout: Implemented `os::processes` on Windows.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
Diff: https://reviews.apache.org/r/46013/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
-----------------------------------------------------------
(Updated April 13, 2016, 10:06 p.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471
Repository: mesos
Description
-------
Stout: Implemented `os::processes` on Windows.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
Diff: https://reviews.apache.org/r/46013/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
-----------------------------------------------------------
(Updated April 13, 2016, 9:49 p.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471
Repository: mesos
Description
-------
Stout: Implemented `os::processes` on Windows.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
Diff: https://reviews.apache.org/r/46013/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
-----------------------------------------------------------
(Updated April 13, 2016, 9:33 p.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471
Repository: mesos
Description
-------
Stout: Implemented `os::processes` on Windows.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
Diff: https://reviews.apache.org/r/46013/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 46013: Stout: Implemented `os::processes` on
Windows.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
-----------------------------------------------------------
(Updated April 11, 2016, 7:19 p.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471
Repository: mesos
Description
-------
Stout: Implemented `os::processes` on Windows.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
Diff: https://reviews.apache.org/r/46013/diff/
Testing
-------
Thanks,
Alex Clemmer