Re: Review Request 46028: Improved comments in SocketManager::next().

[Joris Van Remoortere]

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



Closing this review due to inactivity. Please see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On June 16, 2016, 8:55 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46028/
> ---
> 
> (Updated June 16, 2016, 8:55 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved comments in SocketManager::next().
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> b4cdba6b0cd4f8f373f37118cd2e9d4955f2425a 
> 
> Diff: https://reviews.apache.org/r/46028/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 46028: Improved comments in SocketManager::next().

[Benjamin Mahler]

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



Could you split the patch into two?

```
Documented SocketManager::next.
Updated a stale comment in SocketManager::close.
```


3rdparty/libprocess/src/process.cpp (lines 321 - 325)


How about:

```
// Returns the next message (via an Encoder) to send on the socket, or
// nullptr if there are no more outgoing messages. Must be called only
// after all previously returned messages have been sent. Ownership of
// the Encoder is passed to the caller.
```

I'm not sure how the reader is helped by being informed about the 
implementation details of touching the internal `outgoing` data structure and 
cleaning up some state (it's also likely to grow stale if the internal data 
structures change).



3rdparty/libprocess/src/process.cpp (lines 2026 - 2035)


Your adjustment looks good, but this comment is still stale in that it 
says: 

```
  // Note we
  // need to do this before we call 'sockets.erase(s)' to avoid
  // the potential race with the last reference being in
  // 'sockets'.`"
```

This piece no longer reflects the code accurately (it's from the days when 
we called `::shutdown` directly because `Socket::shutdown` didn't exist, so we 
could potentially call `::shutdown` without keeping a `Socket` reference 
around, in which case we shutdown a stale socket fd!! [1]). It looks as if it 
can be removed in favor of the comment right below this one:

```
  // Hold on to the Socket and remove it from the 'sockets' map so
  // that in the case where 'shutdown()' ends up calling close the
  // termination logic is not run twice.
```

However, this comment is confusing. It looks as if it is also stale for the 
same reason. Now, I'm not sure what the comment is trying to say.. why would 
shutdown call close, how does this lead to running termination logic twice, and 
why does calling this before erasing from the map have anything to do with this 
now that we have `Socket::shutdown`?

Can you check with Joris?

[1] 
https://github.com/apache/mesos/blob/0.22.0/3rdparty/libprocess/src/process.cpp#L1772-L1775


- Benjamin Mahler


On June 16, 2016, 8:55 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46028/
> ---
> 
> (Updated June 16, 2016, 8:55 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved comments in SocketManager::next().
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> b4cdba6b0cd4f8f373f37118cd2e9d4955f2425a 
> 
> Diff: https://reviews.apache.org/r/46028/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 46028: Improved comments in SocketManager::next().

[Neil Conway]

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

(Updated June 16, 2016, 8:55 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebase, nullptr tweak.


Repository: mesos


Description
---

Improved comments in SocketManager::next().


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp b4cdba6b0cd4f8f373f37118cd2e9d4955f2425a 

Diff: https://reviews.apache.org/r/46028/diff/


Testing
---


Thanks,

Neil Conway

Re: Review Request 46028: Improved comments in SocketManager::next().

[Neil Conway]

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

(Updated April 11, 2016, 11:52 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Address review comments.


Repository: mesos


Description
---

Improved comments in SocketManager::next().


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a 

Diff: https://reviews.apache.org/r/46028/diff/


Testing
---


Thanks,

Neil Conway

Re: Review Request 46028: Improved comments in SocketManager::next().

[Neil Conway]

> On April 11, 2016, 10:42 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 1947-1954
> > 
> >
> > The implication of this comment (regardless of your changes) seems to 
> > be that the socket is an outbound socket? Is that true?

This function is called for both inbound and outbound sockets, and AFAICS 
similar comments would apply (i.e., calling `shutdown()` means that any `recv` 
callback will see EOF on the socket). Updated the comment accordingly.


- Neil


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


On April 11, 2016, 2:35 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46028/
> ---
> 
> (Updated April 11, 2016, 2:35 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved comments in SocketManager::next().
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a 
> 
> Diff: https://reviews.apache.org/r/46028/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 46028: Improved comments in SocketManager::next().

[Ben Mahler]

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




3rdparty/libprocess/src/process.cpp (lines 1813 - 1816)


Can you move this comment up to the declaration in the SocketManager 
interface?



3rdparty/libprocess/src/process.cpp (lines 1947 - 1954)


The implication of this comment (regardless of your changes) seems to be 
that the socket is an outbound socket? Is that true?


- Ben Mahler


On April 11, 2016, 2:35 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46028/
> ---
> 
> (Updated April 11, 2016, 2:35 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved comments in SocketManager::next().
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a 
> 
> Diff: https://reviews.apache.org/r/46028/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 46028: Improved comments in SocketManager::next().

[Neil Conway]

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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

Improved comments in SocketManager::next().


Diffs
-

  3rdparty/libprocess/src/process.cpp 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a 

Diff: https://reviews.apache.org/r/46028/diff/


Testing
---


Thanks,

Neil Conway