You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Daniel Pravat <dp...@outlook.com> on 2016/03/02 00:56:11 UTC
Re: Review Request 41632: Windows: Forked signal handling in
`signalhandler.hpp`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41632/
-----------------------------------------------------------
(Updated March 1, 2016, 11:56 p.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Repository: mesos
Description
-------
Windows: Forked signal handling in `signalhandler.hpp`.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/Makefile.am 03eff5a831283f6d298e9a1feecfdc7369cacfe7
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/include/stout/os/signalhandler.hpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/41632/diff/
Testing
-------
OSX: make check
Windows: make
Thanks,
Daniel Pravat
Re: Review Request 41632: Windows: Added Console Ctrl handling in
`slave.cpp`.
Posted by Daniel Pravat <dp...@outlook.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41632/
-----------------------------------------------------------
(Updated April 21, 2016, 8:47 p.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Bugs: MESOS-3644
https://issues.apache.org/jira/browse/MESOS-3644
Repository: mesos
Description
-------
Windows: Added Console Ctrl handling in `slave.cpp`.
Diffs (updated)
-----
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
Re: Review Request 41632: Windows: Added Console Ctrl handling in
`slave.cpp`.
Posted by Daniel Pravat <dp...@outlook.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41632/
-----------------------------------------------------------
(Updated March 8, 2016, 6:15 p.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Bugs: MESOS-3644
https://issues.apache.org/jira/browse/MESOS-3644
Repository: mesos
Description
-------
Windows: Added Console Ctrl handling in `slave.cpp`.
Diffs (updated)
-----
src/slave/posix_signalhandler.hpp PRE-CREATION
src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3
src/slave/windows_ctrlhandler.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/41632/diff/
Testing
-------
OSX: make check
Windows: make
Thanks,
Daniel Pravat
Re: Review Request 41632: Windows: Added Console Ctrl handling in
`slave.cpp`.
Posted by Daniel Pravat <dp...@outlook.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41632/
-----------------------------------------------------------
(Updated March 8, 2016, 6:10 p.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Summary (updated)
-----------------
Windows: Added Console Ctrl handling in `slave.cpp`.
Bugs: MESOS-3644
https://issues.apache.org/jira/browse/MESOS-3644
Repository: mesos
Description (updated)
-------
Windows: Added Console Ctrl handling in `slave.cpp`.
Diffs (updated)
-----
src/slave/posix_signalhandler.hpp PRE-CREATION
src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3
src/slave/windows_ctrlhandler.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/41632/diff/
Testing
-------
OSX: make check
Windows: make
Thanks,
Daniel Pravat
Re: Review Request 41632: Windows: Forked signal handling in
`signalhandler.hpp`.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41632/#review121892
-----------------------------------------------------------
I've left a bunch of comments here, because I'd like us to nudge us towards learning from the style issues we've been having. :)
But, I'd actually like to suggests a different approach.
I agree with Joris that having a `signaledWrapper` inside the header is not ideal. As he said, in general, we should not keep global state in Stout headers, because Stout is an independent API. It is true that `signals.hpp` does call out to the kernel, which affects global state, but it does not itself _maintain_ that state in the header. So to me it would be appropriate to have functions that change the global state (as `signals.hpp` itself does), but not to _introduce_ global state here.
So, I'd like to suggest a couple ways we might change this patch to accomplish these goals. My favorite option is 3.
Before I mention them, I think it's worth considering the history of an API that's pretty successful, which has similar semantics: glog. glog exposes `google::InstallFailureSignalHandler`, which is forked between Windows and POSIX implementations, and which controls the semantics _of that signal handling for modules that are using glog_. I like this because the semantics are controlled by the application that is including glog -- an application can call this function and they will get specific behavior for glog. In our case, we
1. Try to remove the `signaledWrapper` from the `signalhandler.hpp` headers. I'm not sure how to do this, or if it's possible.
2. Perhaps consider making an `attachSignalHandler` API in libprocess. Although it is true that `signals.hpp` is in Stout, libprocess has much more precedent tracking state associated with a process. It could make sense that Stout continue to have `signals.hpp`, but have signal _attaching_ handled by libprocess. It's not entirely clear to me that this is the best place to track this state, but it is a plausible fit.
3. Perhaps put a function like `attachSlaveSigIntHandler` inside the slave. At the `HEAD` of the Windows integration branch it looks like we're using `signalConfigure` exactly once, in the slave. If this is a one-off thing (as it looks like it might be) then I think one option is to just move this directly to the slave package. If we don't need the customizability, then perhaps it's not worth the abstraction?
What do you all think of this? Anything I'm missing?
3rdparty/libprocess/3rdparty/stout/include/Makefile.am (line 94)
<https://reviews.apache.org/r/41632/#comment183741>
Can we please make the `\ ` characters line up in RB (rather than in git).
There are two other instances of this in this file.
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (line 24)
<https://reviews.apache.org/r/41632/#comment183812>
Is this used?
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (line 29)
<https://reviews.apache.org/r/41632/#comment183742>
Mesos project style mandates we do not indent inside a namespace. Can we please pull this back 2 spaces?
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (lines 31 - 33)
<https://reviews.apache.org/r/41632/#comment183755>
Also it seems like this should actually be in an internal namespace? Perhaps `os::internal`?
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (line 32)
<https://reviews.apache.org/r/41632/#comment183751>
Stout uses snake case for everything. In this file, the public symbols at the very least need to change: `signaledWrapper`, `signalHandler`, `configureSignal`, and in the Windows version `CtrlHandler`.
But, please also change the local variables too. :)
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 22)
<https://reviews.apache.org/r/41632/#comment183804>
Is this header used?
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 23)
<https://reviews.apache.org/r/41632/#comment183805>
I think you need `#include <stout/windows.hpp>`, right?
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 25)
<https://reviews.apache.org/r/41632/#comment183746>
Nit: in Mesos we put a space between namespace and code.
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 26)
<https://reviews.apache.org/r/41632/#comment183757>
minor style thing: should be a space between `signaledWrapper` and `CtrlHandler`.
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 27)
<https://reviews.apache.org/r/41632/#comment183747>
Interesting, does this need to be here? Usually we put this kind of stuff in `windows.hpp`.
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 28)
<https://reviews.apache.org/r/41632/#comment183749>
Seems like we actually want this function to go in an internal namespace, like `os::internal`? What do you think?
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 44)
<https://reviews.apache.org/r/41632/#comment183748>
Can we get rid of the trailing whitespace here?
- Alex Clemmer
On March 3, 2016, 3 a.m., Daniel Pravat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41632/
> -----------------------------------------------------------
>
> (Updated March 3, 2016, 3 a.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Bugs: MESOS-3644
> https://issues.apache.org/jira/browse/MESOS-3644
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Windows: Forked signal handling in `signalhandler.hpp`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 03eff5a831283f6d298e9a1feecfdc7369cacfe7
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/signalhandler.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41632/diff/
>
>
> Testing
> -------
>
> OSX: make check
> Windows: make
>
>
> Thanks,
>
> Daniel Pravat
>
>
Re: Review Request 41632: Windows: Forked signal handling in
`signalhandler.hpp`.
Posted by Daniel Pravat <dp...@outlook.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41632/
-----------------------------------------------------------
(Updated March 3, 2016, 3 a.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Bugs: MESOS-3644
https://issues.apache.org/jira/browse/MESOS-3644
Repository: mesos
Description
-------
Windows: Forked signal handling in `signalhandler.hpp`.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/Makefile.am 03eff5a831283f6d298e9a1feecfdc7369cacfe7
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/include/stout/os/signalhandler.hpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/41632/diff/
Testing
-------
OSX: make check
Windows: make
Thanks,
Daniel Pravat
Re: Review Request 41632: Windows: Forked signal handling in
`signalhandler.hpp`.
Posted by Daniel Pravat <dp...@outlook.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41632/
-----------------------------------------------------------
(Updated March 2, 2016, 7:26 a.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Bugs: MESOS-3644
https://issues.apache.org/jira/browse/MESOS-3644
Repository: mesos
Description
-------
Windows: Forked signal handling in `signalhandler.hpp`.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/Makefile.am 03eff5a831283f6d298e9a1feecfdc7369cacfe7
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/include/stout/os/signalhandler.hpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/41632/diff/
Testing
-------
OSX: make check
Windows: make
Thanks,
Daniel Pravat
Re: Review Request 41632: Windows: Forked signal handling in
`signalhandler.hpp`.
Posted by Daniel Pravat <dp...@outlook.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41632/
-----------------------------------------------------------
(Updated March 2, 2016, 12:41 a.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Bugs: MESOS-3644
https://issues.apache.org/jira/browse/MESOS-3644
Repository: mesos
Description
-------
Windows: Forked signal handling in `signalhandler.hpp`.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/Makefile.am 03eff5a831283f6d298e9a1feecfdc7369cacfe7
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/include/stout/os/signalhandler.hpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/41632/diff/
Testing
-------
OSX: make check
Windows: make
Thanks,
Daniel Pravat
Re: Review Request 41632: Windows: Forked signal handling in
`signalhandler.hpp`.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41632/#review121555
-----------------------------------------------------------
It's not clear from the JIRA (https://issues.apache.org/jira/browse/MESOS-3644) what we are using this for.
3rdparty/libprocess/3rdparty/stout/include/Makefile.am (line 94)
<https://reviews.apache.org/r/41632/#comment183274>
style
alphabetize
3rdparty/libprocess/3rdparty/stout/include/Makefile.am (lines 116 - 117)
<https://reviews.apache.org/r/41632/#comment183275>
style
alphabetize
3rdparty/libprocess/3rdparty/stout/include/Makefile.am (lines 134 - 135)
<https://reviews.apache.org/r/41632/#comment183276>
style
alphabetize
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (line 21)
<https://reviews.apache.org/r/41632/#comment183278>
separate block.
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (line 22)
<https://reviews.apache.org/r/41632/#comment183277>
this comes first in a separate block
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (line 26)
<https://reviews.apache.org/r/41632/#comment183281>
can we avoid the typedef?
It's not that big to write out explicitly (We're already doing it in a few places)
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (lines 26 - 28)
<https://reviews.apache.org/r/41632/#comment183283>
How does this library enforce that this is only used in a single module?
This seems extremely dangerous?
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (line 27)
<https://reviews.apache.org/r/41632/#comment183282>
periods at the end of comments.
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (lines 29 - 36)
<https://reviews.apache.org/r/41632/#comment183279>
2 new lines between functions in a namespace
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (line 47)
<https://reviews.apache.org/r/41632/#comment183285>
backticks around `sigaction()`
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp (line 54)
<https://reviews.apache.org/r/41632/#comment183280>
indentation
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (lines 16 - 23)
<https://reviews.apache.org/r/41632/#comment183287>
double includes?
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 24)
<https://reviews.apache.org/r/41632/#comment183289>
separate block
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 25)
<https://reviews.apache.org/r/41632/#comment183288>
this goes by itself at the top
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 28)
<https://reviews.apache.org/r/41632/#comment183290>
periods at the end of comments.
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (lines 28 - 30)
<https://reviews.apache.org/r/41632/#comment183292>
Same question as above. How does the library enforce this single use in a single module?
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 29)
<https://reviews.apache.org/r/41632/#comment183291>
Same typedef concern as above.
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (lines 34 - 48)
<https://reviews.apache.org/r/41632/#comment183293>
style.
Please see other switch statements.
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 47)
<https://reviews.apache.org/r/41632/#comment183294>
indentation?
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (line 54)
<https://reviews.apache.org/r/41632/#comment183295>
style: whitespace
between end of expression and brace. `){` => `) {`
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp (lines 56 - 57)
<https://reviews.apache.org/r/41632/#comment183296>
`} else {`
- Joris Van Remoortere
On March 1, 2016, 11:56 p.m., Daniel Pravat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41632/
> -----------------------------------------------------------
>
> (Updated March 1, 2016, 11:56 p.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Windows: Forked signal handling in `signalhandler.hpp`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 03eff5a831283f6d298e9a1feecfdc7369cacfe7
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/signalhandler.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/signalhandler.hpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41632/diff/
>
>
> Testing
> -------
>
> OSX: make check
> Windows: make
>
>
> Thanks,
>
> Daniel Pravat
>
>