This is an automatically generated e-mail. To reply, visit:

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)

    Can we please make the `\ ` characters line up in RB (rather than in git).
    There are two other instances of this in this file.

(line 24)

    Is this used?

(line 29)

    Mesos project style mandates we do not indent inside a namespace. Can we 
please pull this back 2 spaces?

(lines 31 - 33)

    Also it seems like this should actually be in an internal namespace? 
Perhaps `os::internal`?

(line 32)

    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. :)

(line 22)

    Is this header used?

(line 23)

    I think you need `#include <stout/windows.hpp>`, right?

(line 25)

    Nit: in Mesos we put a space between namespace and code.

(line 26)

    minor style thing: should be a space between `signaledWrapper` and 

(line 27)

    Interesting, does this need to be here? Usually we put this kind of stuff 
in `windows.hpp`.

(line 28)

    Seems like we actually want this function to go in an internal namespace, 
like `os::internal`? What do you think?

(line 44)

    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 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/signalhandler.hpp 
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/signalhandler.hpp 
> Diff: https://reviews.apache.org/r/41632/diff/
> Testing
> -------
> OSX: make check
> Windows: make
> Thanks,
> Daniel Pravat

Reply via email to