mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 49346: Changed the replog's network abstraction to use 'relink'.
Date Wed, 29 Jun 2016 02:38:12 GMT

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


Ship it!




What is a replog?

```
$ grep -R replog mesos | wc -l
$ 0
```

I'm kidding :) but might as well avoid the abbreviation here as I doubt everyone will recognize
it.


src/log/network.hpp (lines 159 - 165)
<https://reviews.apache.org/r/49346/#comment205255>

    I tend to like a newline between each comment "block" (above the NOTE here). I think here
I would be more interested in understanding exactly what a stale socket is. I did some digging
to find pointers and made a suggestion below. The idea is that even though this code isn't
really the right place to document stale connections, it's going to serve as an example of
using RECONNECT and we may consider adopting some of this documentation within the `link`
documentation so that callers understand this pitfall:
    
    ```
        // Link in order to keep a socket open (more efficient).
        //
        // We force a reconnect to avoid sending on a "stale" socket. In
        // general when linking to a remote process, the underlying TCP
        // connection may become "stale". RFC 793 refers to this as a
        // "half-open" connection: the RST is not sent upon the death
        // of the peer and a RST will only be received once further
        // data is sent on the socket.
        // 
        // "Half-open" (aka "stale") connections are typically addressed
        // via keep-alives (see RFC 1122 4.2.3.6) to periodically probe
        // the connection. In our case here we can rely on the re-addition
        // of the network member to force a new connection.
        link(pid, RemoteConnection::RECONNECT);
    ```
    
    Note the `s/Try and/Link in order to/` to make this a bit clearer.


- Benjamin Mahler


On June 29, 2016, 12:48 a.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49346/
> -----------------------------------------------------------
> 
> (Updated June 29, 2016, 12:48 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Hindman, Benjamin Mahler, Gilbert
Song, Artem Harutyunyan, and Jie Yu.
> 
> 
> Bugs: MESOS-5576
>     https://issues.apache.org/jira/browse/MESOS-5576
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/log/network.hpp 56c5dbb38eaeaa70735c47a2266b0dbebc42aa35 
> 
> Diff: https://reviews.apache.org/r/49346/diff/
> 
> 
> Testing
> -------
> 
> The new tests currently fail on Debian 8 and Ubuntu 12, 14, 15, 16.  But pass on CentOS
6, 7.  Also breaks the cmake tests.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>


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