You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@geode.apache.org by Hitesh Khamesra <hk...@pivotal.io> on 2017/08/03 18:59:57 UTC

Review Request 61411: GEODE-3286 Failing to cleanup connections from ConnectionTable receiver table (corrected "stopped" check in previous fix)

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

Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Udo Kohlmeyer, and Brian Rowe.


Repository: geode


Description
-------

In previous fix, we were checking "if thread is stopped then add connection to receiver list". Instead, it should be if thread is stopped then we should not add connection to receiver list. As a fix, we add connection to reciver list if connection is not closed or thread is not stopped.

Added unit for it.


Diffs
-----

  geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java c3ad596 
  geode-core/src/main/java/org/apache/geode/internal/tcp/ConnectionTable.java 69fb7a2 
  geode-core/src/test/java/org/apache/geode/internal/tcp/ConnectionTableTest.java 312c64d 


Diff: https://reviews.apache.org/r/61411/diff/1/


Testing
-------


Thanks,

Hitesh Khamesra

Re: Review Request 61411: GEODE-3286 Failing to cleanup connections from ConnectionTable receiver table (corrected "stopped" check in previous fix)

Posted by Bruce Schuchardt <bs...@pivotal.io>.

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


Ship it!




Ship It!

- Bruce Schuchardt


On Aug. 3, 2017, 4:01 p.m., Hitesh Khamesra wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61411/
> -----------------------------------------------------------
> 
> (Updated Aug. 3, 2017, 4:01 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Udo Kohlmeyer, and Brian Rowe.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> In previous fix, we were checking "if thread is stopped then add connection to receiver list". Instead, it should be if thread is stopped then we should not add connection to receiver list. As a fix, we add connection to reciver list if connection is not closed or thread is not stopped.
> 
> Added unit for it.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java c3ad596 
>   geode-core/src/main/java/org/apache/geode/internal/tcp/ConnectionTable.java 69fb7a2 
>   geode-core/src/test/java/org/apache/geode/internal/tcp/ConnectionTableTest.java 312c64d 
> 
> 
> Diff: https://reviews.apache.org/r/61411/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Hitesh Khamesra
> 
>

Re: Review Request 61411: GEODE-3286 Failing to cleanup connections from ConnectionTable receiver table (corrected "stopped" check in previous fix)

Posted by Udo Kohlmeyer <uk...@gmail.com>.

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


Ship it!




Ship It!

- Udo Kohlmeyer


On Aug. 3, 2017, 11:01 p.m., Hitesh Khamesra wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61411/
> -----------------------------------------------------------
> 
> (Updated Aug. 3, 2017, 11:01 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Udo Kohlmeyer, and Brian Rowe.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> In previous fix, we were checking "if thread is stopped then add connection to receiver list". Instead, it should be if thread is stopped then we should not add connection to receiver list. As a fix, we add connection to reciver list if connection is not closed or thread is not stopped.
> 
> Added unit for it.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java c3ad596 
>   geode-core/src/main/java/org/apache/geode/internal/tcp/ConnectionTable.java 69fb7a2 
>   geode-core/src/test/java/org/apache/geode/internal/tcp/ConnectionTableTest.java 312c64d 
> 
> 
> Diff: https://reviews.apache.org/r/61411/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Hitesh Khamesra
> 
>

Re: Review Request 61411: GEODE-3286 Failing to cleanup connections from ConnectionTable receiver table (corrected "stopped" check in previous fix)

Posted by Alexander Murmann <am...@pivotal.io>.

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


Ship it!




Ship It!

- Alexander Murmann


On Aug. 3, 2017, 11:01 p.m., Hitesh Khamesra wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61411/
> -----------------------------------------------------------
> 
> (Updated Aug. 3, 2017, 11:01 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Udo Kohlmeyer, and Brian Rowe.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> In previous fix, we were checking "if thread is stopped then add connection to receiver list". Instead, it should be if thread is stopped then we should not add connection to receiver list. As a fix, we add connection to reciver list if connection is not closed or thread is not stopped.
> 
> Added unit for it.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java c3ad596 
>   geode-core/src/main/java/org/apache/geode/internal/tcp/ConnectionTable.java 69fb7a2 
>   geode-core/src/test/java/org/apache/geode/internal/tcp/ConnectionTableTest.java 312c64d 
> 
> 
> Diff: https://reviews.apache.org/r/61411/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Hitesh Khamesra
> 
>

Re: Review Request 61411: GEODE-3286 Failing to cleanup connections from ConnectionTable receiver table (corrected "stopped" check in previous fix)

Posted by Hitesh Khamesra <hk...@pivotal.io>.

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

(Updated Aug. 3, 2017, 11:01 p.m.)


Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Udo Kohlmeyer, and Brian Rowe.


Changes
-------

Updated diff based on review


Repository: geode


Description
-------

In previous fix, we were checking "if thread is stopped then add connection to receiver list". Instead, it should be if thread is stopped then we should not add connection to receiver list. As a fix, we add connection to reciver list if connection is not closed or thread is not stopped.

Added unit for it.


Diffs (updated)
-----

  geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java c3ad596 
  geode-core/src/main/java/org/apache/geode/internal/tcp/ConnectionTable.java 69fb7a2 
  geode-core/src/test/java/org/apache/geode/internal/tcp/ConnectionTableTest.java 312c64d 


Diff: https://reviews.apache.org/r/61411/diff/2/

Changes: https://reviews.apache.org/r/61411/diff/1-2/


Testing
-------


Thanks,

Hitesh Khamesra

Re: Review Request 61411: GEODE-3286 Failing to cleanup connections from ConnectionTable receiver table (corrected "stopped" check in previous fix)

Posted by Brian Rowe <br...@pivotal.io>.

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


Fix it, then Ship it!




Nitpicks below, but change looks good.


geode-core/src/test/java/org/apache/geode/internal/tcp/ConnectionTableTest.java
Line 36 (original), 40 (patched)
<https://reviews.apache.org/r/61411/#comment257999>

    Maybe make this @Before instead of calling it at the start of every test.



geode-core/src/test/java/org/apache/geode/internal/tcp/ConnectionTableTest.java
Line 54 (original), 70 (patched)
<https://reviews.apache.org/r/61411/#comment258000>

    Add a when(connection.isReceiverStopped()).thenReturn(false);


- Brian Rowe


On Aug. 3, 2017, 6:59 p.m., Hitesh Khamesra wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61411/
> -----------------------------------------------------------
> 
> (Updated Aug. 3, 2017, 6:59 p.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Udo Kohlmeyer, and Brian Rowe.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> In previous fix, we were checking "if thread is stopped then add connection to receiver list". Instead, it should be if thread is stopped then we should not add connection to receiver list. As a fix, we add connection to reciver list if connection is not closed or thread is not stopped.
> 
> Added unit for it.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java c3ad596 
>   geode-core/src/main/java/org/apache/geode/internal/tcp/ConnectionTable.java 69fb7a2 
>   geode-core/src/test/java/org/apache/geode/internal/tcp/ConnectionTableTest.java 312c64d 
> 
> 
> Diff: https://reviews.apache.org/r/61411/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Hitesh Khamesra
> 
>