mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Clemmer <clemmer.alexan...@gmail.com>
Subject Re: Review Request 41632: Windows: Forked signal handling in `signalhandler.hpp`.
Date Fri, 04 Mar 2016 00:01:08 GMT

-----------------------------------------------------------
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
> 
>


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