Re: Review Request 64911: Refactored connection logic in libprocess.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 55323.

Failed command: `python.exe .\support\apply-reviews.py -n -r 55323`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64911

Relevant logs:

- 
[apply-review-55323-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64911/logs/apply-review-55323-stdout.log):

```
error: patch failed: src/master/master.hpp:2158
error: src/master/master.hpp: patch does not apply
```

- Mesos Reviewbot Windows


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 
> dd4d111c4ae579420060e547dd12f8f0711c 
> 
> 
> Diff: https://reviews.apache.org/r/64911/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 64911: Refactored connection logic in libprocess.

[Benjamin Mahler]

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


newline?



3rdparty/libprocess/src/process.cpp
Lines 1677-1683 (original), 1555-1562 (patched)


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



3rdparty/libprocess/src/process.cpp
Lines 1571-1575 (patched)


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)


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)


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)


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)


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)


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 
> dd4d111c4ae579420060e547dd12f8f0711c 
> 
> 
> Diff: https://reviews.apache.org/r/64911/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>