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 41632: Windows: Added Console Ctrl handling in `slave.cpp`.
Date Tue, 10 May 2016 17:19:46 GMT

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




src/slave/posix_signalhandler.hpp (lines 22 - 50)
<https://reviews.apache.org/r/41632/#comment196684>

    These don't need to be indented.
    You can move everything back 2 spaces.



src/slave/posix_signalhandler.hpp (lines 22 - 23)
<https://reviews.apache.org/r/41632/#comment196701>

    Can we add a comment to both of these files that this is not thread safe?
    You've factored out some specific code, that now can be misused.



src/slave/slave.cpp (line 97)
<https://reviews.apache.org/r/41632/#comment196685>

    missing `.`.



src/slave/slave.cpp (line 101)
<https://reviews.apache.org/r/41632/#comment196686>

    missing `.`.



src/slave/slave.cpp (lines 793 - 794)
<https://reviews.apache.org/r/41632/#comment196692>

    indentation.



src/slave/slave.cpp (lines 793 - 798)
<https://reviews.apache.org/r/41632/#comment196694>

    can we use the `EXIT(EXIT_FAILURE)` codes?



src/slave/slave.cpp (line 798)
<https://reviews.apache.org/r/41632/#comment196693>

    indentation.



src/slave/windows_ctrlhandler.hpp (lines 24 - 56)
<https://reviews.apache.org/r/41632/#comment196695>

    indentation.



src/slave/windows_ctrlhandler.hpp (line 43)
<https://reviews.apache.org/r/41632/#comment196696>

    indentation.



src/slave/windows_ctrlhandler.hpp (lines 46 - 47)
<https://reviews.apache.org/r/41632/#comment196697>

    2 blank lines.



src/slave/windows_ctrlhandler.hpp (lines 51 - 54)
<https://reviews.apache.org/r/41632/#comment196698>

    please leave a space between c-style casts:
    e.g. `(int) val`



src/slave/windows_ctrlhandler.hpp (line 53)
<https://reviews.apache.org/r/41632/#comment196702>

    Did you mean to set `signaledWrapper` to `NULL` here?
    Should we delete it first?


- Joris Van Remoortere


On May 2, 2016, 8:35 p.m., Daniel Pravat wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41632/
> -----------------------------------------------------------
> 
> (Updated May 2, 2016, 8:35 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, Joris Van Remoortere,
and Michael Park.
> 
> 
> Bugs: MESOS-3644
>     https://issues.apache.org/jira/browse/MESOS-3644
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Windows: Added Console Ctrl handling in `slave.cpp`.
> 
> 
> Diffs
> -----
> 
>   src/slave/posix_signalhandler.hpp PRE-CREATION 
>   src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
>   src/slave/windows_ctrlhandler.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41632/diff/
> 
> 
> Testing
> -------
> 
> OSX: make check
> Windows: make
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>


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