mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joris Van Remoortere <joris.van.remoort...@gmail.com>
Subject Re: Review Request 47168: Windows: Implement `kill` and `killtree`.
Date Tue, 10 May 2016 16:17:21 GMT

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




3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (lines 16 - 18)
<https://reviews.apache.org/r/47168/#comment196641>

    Can you group these includes properly please?



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (lines 16 - 23)
<https://reviews.apache.org/r/47168/#comment196643>

    Can you group / order these includes properly please?



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (lines 25 - 26)
<https://reviews.apache.org/r/47168/#comment196644>

    Can you add a comment explaining the purpose of these?



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (line 30)
<https://reviews.apache.org/r/47168/#comment196645>

    - Please use snake_case in stout.
    - `{` braces go on the next line.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (lines 33 - 35)
<https://reviews.apache.org/r/47168/#comment196647>

    Howcome you needed to wrap this with a second set of parentheses?



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (line 35)
<https://reviews.apache.org/r/47168/#comment196646>

    Error messages don't end with a period `.`.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (line 36)
<https://reviews.apache.org/r/47168/#comment196648>

    Can you leave a new line between the log statement, and the return?



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (line 39)
<https://reviews.apache.org/r/47168/#comment196658>

    Why a `shared_ptr` instead of a `unique_ptr`?



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (line 52)
<https://reviews.apache.org/r/47168/#comment196650>

    `{` goes on next line.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (lines 54 - 55)
<https://reviews.apache.org/r/47168/#comment196652>

    Missing periods.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (line 61)
<https://reviews.apache.org/r/47168/#comment196653>

    Missing a `:` after `os::kill()`.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (line 62)
<https://reviews.apache.org/r/47168/#comment196654>

    `Sginal` typo.



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp (lines 65 - 66)
<https://reviews.apache.org/r/47168/#comment196655>

    Can you leave a new line after the `LOG(ERROR)` please?



3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (line 21)
<https://reviews.apache.org/r/47168/#comment196633>

    missing period `.`.
    P < S alphabetize



3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (line 317)
<https://reviews.apache.org/r/47168/#comment196634>

    Not sure why the `.` was removed here?



3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (lines 321 - 323)
<https://reviews.apache.org/r/47168/#comment196635>

    How about:
    ```
    Linux signal flags not used in Windows. We define them per `Linux sys/signal.h` to branch
properly for Windows processes' stop, resume and kill.
    ```



3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (lines 324 - 326)
<https://reviews.apache.org/r/47168/#comment196637>

    Are the `Dec XX` comments correct? They look like they might be in the wrong order?



3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (line 366)
<https://reviews.apache.org/r/47168/#comment196638>

    indentation.



3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (lines 396 - 397)
<https://reviews.apache.org/r/47168/#comment196639>

    Please put `_S_IREAD` and `_S_IWRITE` in backticks.



3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (line 398)
<https://reviews.apache.org/r/47168/#comment196640>

    You added `_O_CREAT`, can you add a comment / explain why?


- Joris Van Remoortere


On May 10, 2016, 8:54 a.m., Daniel Pravat wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47168/
> -----------------------------------------------------------
> 
> (Updated May 10, 2016, 8:54 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van Remoortere, and
Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Windows: Implement `kill` and `killtree`.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 33ddb06e25920096f2d16d4f372129ee2f6a8721

>   3rdparty/libprocess/3rdparty/stout/include/stout/os/kill.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/kill.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/kill.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 3828c53c0dbbf370d642189998af75b9af434e9d

> 
> Diff: https://reviews.apache.org/r/47168/diff/
> 
> 
> Testing
> -------
> 
> Windows: build/test w/ Marathon
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message