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 64911: Refactored connection logic in libprocess.
Date Sun, 14 Jan 2018 21:42:49 GMT

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



Refactoring looks good, but I was curious about a change below that seems to alter the semantics
of RECONNECT and introduces a race where the first socket wins and the RECONNECT loses.

I'm also a little puzzled still at what is preventing two concurrent send loops on a socket.


3rdparty/libprocess/src/process.cpp
Line 1677 (original), 1555 (patched)
<https://reviews.apache.org/r/64911/#comment274572>

    newline?



3rdparty/libprocess/src/process.cpp
Lines 1677-1683 (original), 1555-1562 (patched)
<https://reviews.apache.org/r/64911/#comment274573>

    Just to clarify, this is an existing issue with the old code as well, right?



3rdparty/libprocess/src/process.cpp
Lines 1571-1575 (patched)
<https://reviews.apache.org/r/64911/#comment274574>

    It might not be necessary, but it does seem helpful to spell out that this can happen.



3rdparty/libprocess/src/process.cpp
Lines 1657-1659 (patched)
<https://reviews.apache.org/r/64911/#comment274575>

    Why did you need this? It looks like you're already capturing with = in recover so you
could look at `socket` anyway?



3rdparty/libprocess/src/process.cpp
Lines 1758-1761 (original), 1669-1672 (patched)
<https://reviews.apache.org/r/64911/#comment274576>

    Shouldn't we be CHECKing here that socket.kind is SSL? And maybe also that the future
isFailed?



3rdparty/libprocess/src/process.cpp
Lines 1697-1703 (patched)
<https://reviews.apache.org/r/64911/#comment274577>

    It looks to me like you need to do this in the mutex? Otherwise it seems like the semantics
are changing:
    
    Before: FWICT, a RECONNECT will always replace previous socket that's getting connected.
    
    After: FWICT, we check here under the mutex that socket is still contained, then RECONNECT
swaps it, then we swap it again here on the first connect socket. So the first socket can
win the race?



3rdparty/libprocess/src/process.cpp
Lines 1805-1822 (original), 1714-1738 (patched)
<https://reviews.apache.org/r/64911/#comment274578>

    Hm.. I was expecting `connect` to return once connected, but this .then returning the
receive loop makes it look like `connect` will return after the receive loop finishes reading..?
Am I mis-reading this?



3rdparty/libprocess/src/socket_manager.hpp
Lines 53-54 (patched)
<https://reviews.apache.org/r/64911/#comment274571>

    Hm.. this could use some more detail about what a downgrade is and when we downgrade?
E.g.
    
    ```
    Helper to connect the socket to the provided address.
    If the initial connection attempt was done over SSL and
    fails, we will attempt to "downgrade" the connection by
    re-connecting without SSL. The "downgrade" can be
    enabled or disabled via a flag / environment variable.
    ```


- Benjamin Mahler


On Jan. 3, 2018, 7:23 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64911/
> -----------------------------------------------------------
> 
> (Updated Jan. 3, 2018, 7:23 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Refactored connection logic in libprocess.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/process.cpp 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf 
>   3rdparty/libprocess/src/socket_manager.hpp dd4d111c4ae579420060e547d1111d12f8f0711c

> 
> 
> Diff: https://reviews.apache.org/r/64911/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>


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