Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49837, 49844]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On July 17, 2016, 9:15 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49844/
> ---
> 
> (Updated July 17, 2016, 9:15 p.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-5812
> https://issues.apache.org/jira/browse/MESOS-5812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This FD leak would only surface when running tests. We hold on to
> a reference of the `Connection` object in the client so that it is
> not destroyed while the connection is active. When running tests,
> the IP:Port of libprocess remain the same which means the objects
> keep on accumulating. In a real world cluster, we remove the
> subscriber upon noticing a _disconnection_ i.e. this means the
> socket has already been already closed upstream by Libprocess on
> the server side.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ac998b1f5b305a9bff9d9e5cd205a6c3481f9b38 
>   src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 
> 
> Diff: https://reviews.apache.org/r/49844/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=1000) no FD leaks
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Anand Mazumdar]

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

(Updated July 17, 2016, 9:15 p.m.)


Review request for mesos, Vinod Kone and Zhitao Li.


Changes
---

Added a comment around making `HttpConnection` a RAII class. NNFR


Bugs: MESOS-5812
https://issues.apache.org/jira/browse/MESOS-5812


Repository: mesos


Description (updated)
---

This FD leak would only surface when running tests. We hold on to
a reference of the `Connection` object in the client so that it is
not destroyed while the connection is active. When running tests,
the IP:Port of libprocess remain the same which means the objects
keep on accumulating. In a real world cluster, we remove the
subscriber upon noticing a _disconnection_ i.e. this means the
socket has already been already closed upstream by Libprocess on
the server side.


Diffs (updated)
-

  src/master/master.hpp ac998b1f5b305a9bff9d9e5cd205a6c3481f9b38 
  src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 

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


Testing
---

make check (gtest_repeat=1000) no FD leaks


Thanks,

Anand Mazumdar

Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49837, 49844]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On July 12, 2016, 1:25 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49844/
> ---
> 
> (Updated July 12, 2016, 1:25 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-5812
> https://issues.apache.org/jira/browse/MESOS-5812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This FD leak would only surface when running tests. We hold on to
> a reference of the `Connection` object in the client so that it is
> not destroyed before the connection is active. When running tests,
> the IP:Port of libprocess remain the same which means the objects
> keep on accumulating. In a real world cluster, we remove the
> subscriber upon noticing a _disconnection_ i.e. this means the
> socket has already been already closed upstream by Libprocess on
> the server side.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 845f2f6103b58e114dc5d50e3fcf70607c92a469 
>   src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 
> 
> Diff: https://reviews.apache.org/r/49844/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=1000) no FD leaks
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Vinod Kone]

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


Fix it, then Ship it!





src/master/master.hpp (line 1758)


As discussed offline, lets add a TODO to make `HttpConnection` an RAII 
object instead.


- Vinod Kone


On July 12, 2016, 1:25 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49844/
> ---
> 
> (Updated July 12, 2016, 1:25 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-5812
> https://issues.apache.org/jira/browse/MESOS-5812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This FD leak would only surface when running tests. We hold on to
> a reference of the `Connection` object in the client so that it is
> not destroyed before the connection is active. When running tests,
> the IP:Port of libprocess remain the same which means the objects
> keep on accumulating. In a real world cluster, we remove the
> subscriber upon noticing a _disconnection_ i.e. this means the
> socket has already been already closed upstream by Libprocess on
> the server side.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 845f2f6103b58e114dc5d50e3fcf70607c92a469 
>   src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 
> 
> Diff: https://reviews.apache.org/r/49844/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=1000) no FD leaks
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Anand Mazumdar]

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

(Updated July 12, 2016, 1:25 a.m.)


Review request for mesos, Vinod Kone and Zhitao Li.


Changes
---

Review comments


Bugs: MESOS-5812
https://issues.apache.org/jira/browse/MESOS-5812


Repository: mesos


Description
---

This FD leak would only surface when running tests. We hold on to
a reference of the `Connection` object in the client so that it is
not destroyed before the connection is active. When running tests,
the IP:Port of libprocess remain the same which means the objects
keep on accumulating. In a real world cluster, we remove the
subscriber upon noticing a _disconnection_ i.e. this means the
socket has already been already closed upstream by Libprocess on
the server side.


Diffs (updated)
-

  src/master/master.hpp 845f2f6103b58e114dc5d50e3fcf70607c92a469 
  src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 

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


Testing
---

make check (gtest_repeat=1000) no FD leaks


Thanks,

Anand Mazumdar

Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Anand Mazumdar]

> On July 9, 2016, 7:21 p.m., Vinod Kone wrote:
> > src/master/master.hpp, line 1765
> > 
> >
> > don't follow why this has to be Owned.
> 
> Anand Mazumdar wrote:
> Same reasoning as above. If it's not an `Owned`, it would be destroyed 
> when the stack allocated object (`Subscriber`) goes out of scope leading to 
> the `Pipe` getting closed.
> 
> Vinod Kone wrote:
> Why is ~Subscriber getting called when the subscriber is removed from the 
> map a problem? I might be missing something here.
> 
> Anand Mazumdar wrote:
> Let me elaborate a bit. 
> 
> `~Subscriber()` getting called when the subscriber is removed from the 
> map is not an issue. Infact, that is the desired behavior.
> 
> The problem however is `~Subscriber()` _also_ getting called for the 
> temporary objects on the stack and thereby un-intentinally closing the `Pipe` 
> i.e.
> 
> - https://github.com/apache/mesos/blob/master/src/master/master.cpp#L7595
> - https://github.com/apache/mesos/blob/master/src/master/master.cpp#L7576
> 
> Vinod Kone wrote:
> hmm. I see. my question then would be whether automatically doing 
> http.close() in ~Subscriber is safe? What is the guarantee that someone won't 
> come along in the future takes a copy of Subscriber? Should we disallow 
> copy/assignment ?

Agreed. Explicitly disallowing would be a good idea to avoid such potential 
gotchas in the future. I would update the patch shortly.


- Anand


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


On July 9, 2016, 3:21 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49844/
> ---
> 
> (Updated July 9, 2016, 3:21 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-5812
> https://issues.apache.org/jira/browse/MESOS-5812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This FD leak would only surface when running tests. We hold on to
> a reference of the `Connection` object in the client so that it is
> not destroyed before the connection is active. When running tests,
> the IP:Port of libprocess remain the same which means the objects
> keep on accumulating. In a real world cluster, we remove the
> subscriber upon noticing a _disconnection_ i.e. this means the
> socket has already been already closed upstream by Libprocess on
> the server side.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 845f2f6103b58e114dc5d50e3fcf70607c92a469 
>   src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 
> 
> Diff: https://reviews.apache.org/r/49844/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=1000) no FD leaks
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Vinod Kone]

> On July 9, 2016, 7:21 p.m., Vinod Kone wrote:
> > src/master/master.hpp, line 1765
> > 
> >
> > don't follow why this has to be Owned.
> 
> Anand Mazumdar wrote:
> Same reasoning as above. If it's not an `Owned`, it would be destroyed 
> when the stack allocated object (`Subscriber`) goes out of scope leading to 
> the `Pipe` getting closed.
> 
> Vinod Kone wrote:
> Why is ~Subscriber getting called when the subscriber is removed from the 
> map a problem? I might be missing something here.
> 
> Anand Mazumdar wrote:
> Let me elaborate a bit. 
> 
> `~Subscriber()` getting called when the subscriber is removed from the 
> map is not an issue. Infact, that is the desired behavior.
> 
> The problem however is `~Subscriber()` _also_ getting called for the 
> temporary objects on the stack and thereby un-intentinally closing the `Pipe` 
> i.e.
> 
> - https://github.com/apache/mesos/blob/master/src/master/master.cpp#L7595
> - https://github.com/apache/mesos/blob/master/src/master/master.cpp#L7576

hmm. I see. my question then would be whether automatically doing http.close() 
in ~Subscriber is safe? What is the guarantee that someone won't come along in 
the future takes a copy of Subscriber? Should we disallow copy/assignment ?


- Vinod


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


On July 9, 2016, 3:21 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49844/
> ---
> 
> (Updated July 9, 2016, 3:21 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-5812
> https://issues.apache.org/jira/browse/MESOS-5812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This FD leak would only surface when running tests. We hold on to
> a reference of the `Connection` object in the client so that it is
> not destroyed before the connection is active. When running tests,
> the IP:Port of libprocess remain the same which means the objects
> keep on accumulating. In a real world cluster, we remove the
> subscriber upon noticing a _disconnection_ i.e. this means the
> socket has already been already closed upstream by Libprocess on
> the server side.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 845f2f6103b58e114dc5d50e3fcf70607c92a469 
>   src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 
> 
> Diff: https://reviews.apache.org/r/49844/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=1000) no FD leaks
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Anand Mazumdar]

> On July 9, 2016, 7:21 p.m., Vinod Kone wrote:
> > src/master/master.hpp, line 1765
> > 
> >
> > don't follow why this has to be Owned.
> 
> Anand Mazumdar wrote:
> Same reasoning as above. If it's not an `Owned`, it would be destroyed 
> when the stack allocated object (`Subscriber`) goes out of scope leading to 
> the `Pipe` getting closed.
> 
> Vinod Kone wrote:
> Why is ~Subscriber getting called when the subscriber is removed from the 
> map a problem? I might be missing something here.

Let me elaborate a bit. 

`~Subscriber()` getting called when the subscriber is removed from the map is 
not an issue. Infact, that is the desired behavior.

The problem however is `~Subscriber()` _also_ getting called for the temporary 
objects on the stack and thereby un-intentinally closing the `Pipe` i.e.

- https://github.com/apache/mesos/blob/master/src/master/master.cpp#L7595
- https://github.com/apache/mesos/blob/master/src/master/master.cpp#L7576


- Anand


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


On July 9, 2016, 3:21 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49844/
> ---
> 
> (Updated July 9, 2016, 3:21 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-5812
> https://issues.apache.org/jira/browse/MESOS-5812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This FD leak would only surface when running tests. We hold on to
> a reference of the `Connection` object in the client so that it is
> not destroyed before the connection is active. When running tests,
> the IP:Port of libprocess remain the same which means the objects
> keep on accumulating. In a real world cluster, we remove the
> subscriber upon noticing a _disconnection_ i.e. this means the
> socket has already been already closed upstream by Libprocess on
> the server side.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 845f2f6103b58e114dc5d50e3fcf70607c92a469 
>   src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 
> 
> Diff: https://reviews.apache.org/r/49844/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=1000) no FD leaks
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Vinod Kone]

> On July 9, 2016, 7:21 p.m., Vinod Kone wrote:
> > src/master/master.hpp, line 1765
> > 
> >
> > don't follow why this has to be Owned.
> 
> Anand Mazumdar wrote:
> Same reasoning as above. If it's not an `Owned`, it would be destroyed 
> when the stack allocated object (`Subscriber`) goes out of scope leading to 
> the `Pipe` getting closed.

Why is ~Subscriber getting called when the subscriber is removed from the map a 
problem? I might be missing something here.


- Vinod


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


On July 9, 2016, 3:21 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49844/
> ---
> 
> (Updated July 9, 2016, 3:21 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-5812
> https://issues.apache.org/jira/browse/MESOS-5812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This FD leak would only surface when running tests. We hold on to
> a reference of the `Connection` object in the client so that it is
> not destroyed before the connection is active. When running tests,
> the IP:Port of libprocess remain the same which means the objects
> keep on accumulating. In a real world cluster, we remove the
> subscriber upon noticing a _disconnection_ i.e. this means the
> socket has already been already closed upstream by Libprocess on
> the server side.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 845f2f6103b58e114dc5d50e3fcf70607c92a469 
>   src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 
> 
> Diff: https://reviews.apache.org/r/49844/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=1000) no FD leaks
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Anand Mazumdar]

> On July 9, 2016, 7:21 p.m., Vinod Kone wrote:
> > src/master/master.hpp, line 1754
> > 
> >
> > should this be done in ~HttpConnection instead?

hmm, `HttpConnection` is not meant to be used as an RAII object. We copy it by 
value a lot and hence it exposes a `close()` to facilitate explicitly closing 
it.


> On July 9, 2016, 7:21 p.m., Vinod Kone wrote:
> > src/master/master.hpp, line 1765
> > 
> >
> > don't follow why this has to be Owned.

Same reasoning as above. If it's not an `Owned`, it would be destroyed when the 
stack allocated object (`Subscriber`) goes out of scope leading to the `Pipe` 
getting closed.


- Anand


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


On July 9, 2016, 3:21 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49844/
> ---
> 
> (Updated July 9, 2016, 3:21 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-5812
> https://issues.apache.org/jira/browse/MESOS-5812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This FD leak would only surface when running tests. We hold on to
> a reference of the `Connection` object in the client so that it is
> not destroyed before the connection is active. When running tests,
> the IP:Port of libprocess remain the same which means the objects
> keep on accumulating. In a real world cluster, we remove the
> subscriber upon noticing a _disconnection_ i.e. this means the
> socket has already been already closed upstream by Libprocess on
> the server side.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 845f2f6103b58e114dc5d50e3fcf70607c92a469 
>   src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 
> 
> Diff: https://reviews.apache.org/r/49844/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=1000) no FD leaks
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Vinod Kone]

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




src/master/master.hpp (line 1754)


should this be done in ~HttpConnection instead?



src/master/master.hpp (line 1765)


don't follow why this has to be Owned.


- Vinod Kone


On July 9, 2016, 3:21 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49844/
> ---
> 
> (Updated July 9, 2016, 3:21 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-5812
> https://issues.apache.org/jira/browse/MESOS-5812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This FD leak would only surface when running tests. We hold on to
> a reference of the `Connection` object in the client so that it is
> not destroyed before the connection is active. When running tests,
> the IP:Port of libprocess remain the same which means the objects
> keep on accumulating. In a real world cluster, we remove the
> subscriber upon noticing a _disconnection_ i.e. this means the
> socket has already been already closed upstream by Libprocess on
> the server side.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 845f2f6103b58e114dc5d50e3fcf70607c92a469 
>   src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 
> 
> Diff: https://reviews.apache.org/r/49844/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=1000) no FD leaks
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49837, 49844]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On July 9, 2016, 3:21 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49844/
> ---
> 
> (Updated July 9, 2016, 3:21 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-5812
> https://issues.apache.org/jira/browse/MESOS-5812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This FD leak would only surface when running tests. We hold on to
> a reference of the `Connection` object in the client so that it is
> not destroyed before the connection is active. When running tests,
> the IP:Port of libprocess remain the same which means the objects
> keep on accumulating. In a real world cluster, we remove the
> subscriber upon noticing a _disconnection_ i.e. this means the
> socket has already been already closed upstream by Libprocess on
> the server side.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 845f2f6103b58e114dc5d50e3fcf70607c92a469 
>   src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 
> 
> Diff: https://reviews.apache.org/r/49844/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=1000) no FD leaks
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Zhitao Li]

> On July 9, 2016, 4:22 a.m., Zhitao Li wrote:
> > src/master/master.cpp, lines 7602-7610
> > 
> >
> > Does the order of these two statements matter (e.g. we need ot ensure 
> > `subscribers.subscribed` always has the streamId in it before close)?
> 
> Anand Mazumdar wrote:
> We emit a warning if the subscriber is not part of the `subscribers` map. 
> Currently, it's part of a `defer()`, so it shouldn't matter. Are you alluding 
> to the fact that it would be more _readable_ if we reorder these operations?

Yes, reorder these two makes the code more readable to me.


- Zhitao


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


On July 9, 2016, 3:21 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49844/
> ---
> 
> (Updated July 9, 2016, 3:21 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-5812
> https://issues.apache.org/jira/browse/MESOS-5812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This FD leak would only surface when running tests. We hold on to
> a reference of the `Connection` object in the client so that it is
> not destroyed before the connection is active. When running tests,
> the IP:Port of libprocess remain the same which means the objects
> keep on accumulating. In a real world cluster, we remove the
> subscriber upon noticing a _disconnection_ i.e. this means the
> socket has already been already closed upstream by Libprocess on
> the server side.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 845f2f6103b58e114dc5d50e3fcf70607c92a469 
>   src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 
> 
> Diff: https://reviews.apache.org/r/49844/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=1000) no FD leaks
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Anand Mazumdar]

> On July 9, 2016, 4:22 a.m., Zhitao Li wrote:
> > src/master/master.cpp, lines 7602-7610
> > 
> >
> > Does the order of these two statements matter (e.g. we need ot ensure 
> > `subscribers.subscribed` always has the streamId in it before close)?

We emit a warning if the subscriber is not part of the `subscribers` map. 
Currently, it's part of a `defer()`, so it shouldn't matter. Are you alluding 
to the fact that it would be more _readable_ if we reorder these operations?


- Anand


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


On July 9, 2016, 3:21 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49844/
> ---
> 
> (Updated July 9, 2016, 3:21 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-5812
> https://issues.apache.org/jira/browse/MESOS-5812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This FD leak would only surface when running tests. We hold on to
> a reference of the `Connection` object in the client so that it is
> not destroyed before the connection is active. When running tests,
> the IP:Port of libprocess remain the same which means the objects
> keep on accumulating. In a real world cluster, we remove the
> subscriber upon noticing a _disconnection_ i.e. this means the
> socket has already been already closed upstream by Libprocess on
> the server side.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 845f2f6103b58e114dc5d50e3fcf70607c92a469 
>   src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 
> 
> Diff: https://reviews.apache.org/r/49844/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=1000) no FD leaks
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Zhitao Li]

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




src/master/master.cpp (lines 7598 - 7606)


Does the order of these two statements matter (e.g. we need ot ensure 
`subscribers.subscribed` always has the streamId in it before close)?


- Zhitao Li


On July 9, 2016, 3:21 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49844/
> ---
> 
> (Updated July 9, 2016, 3:21 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-5812
> https://issues.apache.org/jira/browse/MESOS-5812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This FD leak would only surface when running tests. We hold on to
> a reference of the `Connection` object in the client so that it is
> not destroyed before the connection is active. When running tests,
> the IP:Port of libprocess remain the same which means the objects
> keep on accumulating. In a real world cluster, we remove the
> subscriber upon noticing a _disconnection_ i.e. this means the
> socket has already been already closed upstream by Libprocess on
> the server side.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 845f2f6103b58e114dc5d50e3fcf70607c92a469 
>   src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 
> 
> Diff: https://reviews.apache.org/r/49844/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=1000) no FD leaks
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>