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 49177: Added 'relink' semantics to ProcessBase::link.
Date Mon, 27 Jun 2016 22:41:42 GMT

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


Fix it, then Ship it!





3rdparty/libprocess/include/process/process.hpp (lines 153 - 174)
<https://reviews.apache.org/r/49177/#comment204907>

    How about:
    
    ```
    enum class RemoteConnection {
      REUSE,
      RECONNECT,
    }
    
    link(master, RemoteConnection::REUSE);
    link(master, RemoteConnection::RECONNECT);
    ```
    
    Be sure to update the documentation accordingly. One suggestion is to avoid discussing
remote vs local within the enum value documentation and instead mention at the top that the
enum describes the remote link semantics but has no effect on the local links (because as
described below, they are guaranteed to provide perfect monitoring).



3rdparty/libprocess/src/process.cpp (line 1594)
<https://reviews.apache.org/r/49177/#comment204913>

    How about:
    
    ```
    Socket existing = ...;
    // or
    const Socket existing = ...;
    ```
    
    Consider CHECKing the invariant that the socket map contains the fd, or using `at`:
    
    ```
    CHECK(sockets.count(persists.at(to.address) == 1);
    Socket existing = sockets[persists[to.address]];
    
    // or
    Socket existing = sockets.at(persists.at(to.address));
    ```



3rdparty/libprocess/src/process.cpp (lines 1600 - 1604)
<https://reviews.apache.org/r/49177/#comment204936>

    Whoops, this needs to still run in the local case. The tests should fail?



3rdparty/libprocess/src/process.cpp (line 1610)
<https://reviews.apache.org/r/49177/#comment204920>

    If you want to clean this up (and the same code in link_connect) in a separate patch (no
need for to send a review), can do the following:
    
    ```
    socket->connect(to.address)
    ```


- Benjamin Mahler


On June 24, 2016, 2:43 a.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49177/
> -----------------------------------------------------------
> 
> (Updated June 24, 2016, 2:43 a.m.)
> 
> 
> Review request for mesos, 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
> -------
> 
> The `REMOTE_RELINK` option for `ProcessBase::link` will force the
> `SocketManager` to create a new socket if a persistent link already
> exists.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/process.hpp 2946e50081c4a418a868083806a26f995c295007

>   3rdparty/libprocess/src/process.cpp 9bae71246e751e491be5a989eea8aca29c9aa751 
> 
> Diff: https://reviews.apache.org/r/49177/diff/
> 
> 
> Testing
> -------
> 
> make check (OSX)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>


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