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 2016/10/21 17:57:09 UTC
Review Request 53092: GEODE-2011 Client clears pdx registry needs
synchronization
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53092/
-----------------------------------------------------------
Review request for geode, Bruce Schuchardt, Udo Kohlmeyer, and Dan Smith.
Repository: geode
Description
-------
We clear pdx registry in client when we re-connect to cluster. But during that time other thread may end up using old registry while other thread is clearing this. Thus we need to synchronize that. In current code it happens through listener events and I modified that notification to synchronize this.
Diffs
-----
geode-core/src/main/java/org/apache/geode/cache/client/internal/EndpointManagerImpl.java 3f3d725
Diff: https://reviews.apache.org/r/53092/diff/
Testing
-------
Thanks,
Hitesh Khamesra
Re: Review Request 53092: GEODE-2011 Client clears pdx registry needs
synchronization
Posted by Hitesh Khamesra <hk...@pivotal.io>.
> On Oct. 21, 2016, 6:28 p.m., Darrel Schneider wrote:
> > geode-core/src/main/java/org/apache/geode/cache/client/internal/EndpointManagerImpl.java, line 74
> > <https://reviews.apache.org/r/53092/diff/1/?file=1543174#file1543174line74>
> >
> > this code finds that it currently has no endpoint (endpoint == null) or the one it had has been closed (endpoint.isClosed). In both those cases you have it call listener.clearPdxRegistry which then calls PdxRegistryRecoveryListener.endPointNowInUse.
> >
> > The does not make logical sense to me.
> > The endpoint that is now in use would be the one created on line 76, the new endpoint you are adding. The PdxRegistryRecoveryListener does not even use the "endpoint" argument but I wonder if you should be doing this on line 77 with the new endpoint.
Right, it should be on line 77. Thanks
- Hitesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53092/#review153572
-----------------------------------------------------------
On Oct. 21, 2016, 5:57 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53092/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2016, 5:57 p.m.)
>
>
> Review request for geode, Bruce Schuchardt, Udo Kohlmeyer, and Dan Smith.
>
>
> Repository: geode
>
>
> Description
> -------
>
> We clear pdx registry in client when we re-connect to cluster. But during that time other thread may end up using old registry while other thread is clearing this. Thus we need to synchronize that. In current code it happens through listener events and I modified that notification to synchronize this.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/cache/client/internal/EndpointManagerImpl.java 3f3d725
>
> Diff: https://reviews.apache.org/r/53092/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 53092: GEODE-2011 Client clears pdx registry needs
synchronization
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53092/#review153572
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/cache/client/internal/EndpointManagerImpl.java (line 74)
<https://reviews.apache.org/r/53092/#comment222966>
this code finds that it currently has no endpoint (endpoint == null) or the one it had has been closed (endpoint.isClosed). In both those cases you have it call listener.clearPdxRegistry which then calls PdxRegistryRecoveryListener.endPointNowInUse.
The does not make logical sense to me.
The endpoint that is now in use would be the one created on line 76, the new endpoint you are adding. The PdxRegistryRecoveryListener does not even use the "endpoint" argument but I wonder if you should be doing this on line 77 with the new endpoint.
- Darrel Schneider
On Oct. 21, 2016, 10:57 a.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53092/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2016, 10:57 a.m.)
>
>
> Review request for geode, Bruce Schuchardt, Udo Kohlmeyer, and Dan Smith.
>
>
> Repository: geode
>
>
> Description
> -------
>
> We clear pdx registry in client when we re-connect to cluster. But during that time other thread may end up using old registry while other thread is clearing this. Thus we need to synchronize that. In current code it happens through listener events and I modified that notification to synchronize this.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/cache/client/internal/EndpointManagerImpl.java 3f3d725
>
> Diff: https://reviews.apache.org/r/53092/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 53092: GEODE-2011 Client clears pdx registry needs
synchronization
Posted by Hitesh Khamesra <hk...@pivotal.io>.
> On Oct. 21, 2016, 10:25 p.m., Bruce Schuchardt wrote:
> > is it possible to unit test this change? PdxClientServerDUnitTest.testNonPersistentServerRestartAutoSerializer ran into the problem but seems to be marked Flaky. Can you modify that test to always hit the problem and remove the Flaky label?
Possibly we can do that by introducing some testHook and call from PdxRegistoryRecoveryListener and then just wait "before clearing the pdx Registry".
- Hitesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53092/#review153605
-----------------------------------------------------------
On Oct. 21, 2016, 6:52 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53092/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2016, 6:52 p.m.)
>
>
> Review request for geode, Bruce Schuchardt, Udo Kohlmeyer, and Dan Smith.
>
>
> Repository: geode
>
>
> Description
> -------
>
> We clear pdx registry in client when we re-connect to cluster. But during that time other thread may end up using old registry while other thread is clearing this. Thus we need to synchronize that. In current code it happens through listener events and I modified that notification to synchronize this.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/cache/client/internal/EndpointManagerImpl.java 3f3d725
>
> Diff: https://reviews.apache.org/r/53092/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 53092: GEODE-2011 Client clears pdx registry needs
synchronization
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53092/#review153605
-----------------------------------------------------------
is it possible to unit test this change? PdxClientServerDUnitTest.testNonPersistentServerRestartAutoSerializer ran into the problem but seems to be marked Flaky. Can you modify that test to always hit the problem and remove the Flaky label?
- Bruce Schuchardt
On Oct. 21, 2016, 6:52 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53092/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2016, 6:52 p.m.)
>
>
> Review request for geode, Bruce Schuchardt, Udo Kohlmeyer, and Dan Smith.
>
>
> Repository: geode
>
>
> Description
> -------
>
> We clear pdx registry in client when we re-connect to cluster. But during that time other thread may end up using old registry while other thread is clearing this. Thus we need to synchronize that. In current code it happens through listener events and I modified that notification to synchronize this.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/cache/client/internal/EndpointManagerImpl.java 3f3d725
>
> Diff: https://reviews.apache.org/r/53092/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 53092: GEODE-2011 Client clears pdx registry needs
synchronization
Posted by Hitesh Khamesra <hk...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53092/
-----------------------------------------------------------
(Updated Oct. 21, 2016, 6:52 p.m.)
Review request for geode, Bruce Schuchardt, Udo Kohlmeyer, and Dan Smith.
Changes
-------
Updated diff
Repository: geode
Description
-------
We clear pdx registry in client when we re-connect to cluster. But during that time other thread may end up using old registry while other thread is clearing this. Thus we need to synchronize that. In current code it happens through listener events and I modified that notification to synchronize this.
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/cache/client/internal/EndpointManagerImpl.java 3f3d725
Diff: https://reviews.apache.org/r/53092/diff/
Testing
-------
Thanks,
Hitesh Khamesra