You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by xiaojian zhou <zh...@gmail.com> on 2016/03/25 18:59:56 UTC
Review Request 45336: GEODE-920: Not to create CCN for gateway
receiver
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45336/
-----------------------------------------------------------
Review request for geode, anilkumar gingade and Dan Smith.
Bugs: GEM-454, GEM-525 and GEODE-920
https://issues.apache.org/jira/browse/GEM-454
https://issues.apache.org/jira/browse/GEM-525
https://issues.apache.org/jira/browse/GEODE-920
Repository: geode
Description
-------
It's both GEODE-920, GEM-454, GEM-525.
If gateway receiver is created before cache server, the notify subscriptions attributes can never been set, since CCN singleton is created by the gateway receiver.
The easy fix is not to let gateway receiver create the CCN singleton.
Diffs
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/InitialImageOperation.java 34e3855
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ha/HAContainerRegion.java 8803c32
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/AcceptorImpl.java 9f18f50
geode-wan/src/test/java/com/gemstone/gemfire/internal/cache/wan/CacheClientNotifierDUnitTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/45336/diff/
Testing
-------
- introduced new dunit test.
- precheckin
- regression tests:
Thanks,
xiaojian zhou
Re: Review Request 45336: GEODE-920: Not to create CCN for gateway
receiver
Posted by anilkumar gingade <ag...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45336/#review126608
-----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/InitialImageOperation.java (line 2182)
<https://reviews.apache.org/r/45336/#comment189624>
Since its debug level log...we can keep it simple by having only one condition...
if (ccn != null && ccn.getHaContainer() != null) {
//log...
}
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/InitialImageOperation.java (line 3713)
<https://reviews.apache.org/r/45336/#comment189627>
We could move this to, below try catch block...
CacheClientProxy proxy;
HaContainerWrapper container;
if (ccn != null) {
container = ccn.getHAContainer();
if (container != null) {
proxy = container.getProxy();
}
}
And below we do log the message if proxy is null...in that we could log values of ccn and hacontainer...that will tell why proxy is null...
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/AcceptorImpl.java (line 533)
<https://reviews.apache.org/r/45336/#comment189641>
We should create HA contianer in CacheClientNotifier.getInstance() than here...As the container is owned by CCN.
Moving to getInstance() keeps the HAContainer creating thread safe...
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/CacheClientNotifier.java (line 2152)
<https://reviews.apache.org/r/45336/#comment189642>
We can change this to:
if (!gateway) {
init()
}
No need to set it to null...as hacontainer is no more final...
- anilkumar gingade
On April 1, 2016, 12:37 a.m., xiaojian zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45336/
> -----------------------------------------------------------
>
> (Updated April 1, 2016, 12:37 a.m.)
>
>
> Review request for geode, anilkumar gingade and Dan Smith.
>
>
> Bugs: GEM-454, GEM-525 and GEODE-920
> https://issues.apache.org/jira/browse/GEM-454
> https://issues.apache.org/jira/browse/GEM-525
> https://issues.apache.org/jira/browse/GEODE-920
>
>
> Repository: geode
>
>
> Description
> -------
>
> It's both GEODE-920, GEM-454, GEM-525.
>
> If gateway receiver is created before cache server, the notify subscriptions attributes can never been set, since CCN singleton is created by the gateway receiver.
>
> The easy fix is not to let gateway receiver create the CCN singleton.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/InitialImageOperation.java 9bd3faf
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java 95616fe
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ha/HAContainerRegion.java 3bed769
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/AcceptorImpl.java 5b20e86
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/CacheClientNotifier.java 6ac4690
> geode-wan/src/test/java/com/gemstone/gemfire/internal/cache/wan/CacheClientNotifierDUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45336/diff/
>
>
> Testing
> -------
>
> - introduced new dunit test.
> - precheckin
> - regression tests:
>
>
> Thanks,
>
> xiaojian zhou
>
>
Re: Review Request 45336: GEODE-920: Not to create CCN for gateway
receiver
Posted by anilkumar gingade <ag...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45336/#review126883
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/CacheClientNotifier.java (line 164)
<https://reviews.apache.org/r/45336/#comment189972>
Can we move creating HAContainer here...That way it will be called in one place under a signle condition....
E.g.:
if (ccn == null ) {
// create ccn
}
if (!isGateWayReciver && haContainer == null)
{
// create HAContainer.
}
return.
We can remove the logic creating HAContainer in CCN constructor....
- anilkumar gingade
On April 1, 2016, 10:11 p.m., xiaojian zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45336/
> -----------------------------------------------------------
>
> (Updated April 1, 2016, 10:11 p.m.)
>
>
> Review request for geode, anilkumar gingade and Dan Smith.
>
>
> Bugs: GEM-454, GEM-525 and GEODE-920
> https://issues.apache.org/jira/browse/GEM-454
> https://issues.apache.org/jira/browse/GEM-525
> https://issues.apache.org/jira/browse/GEODE-920
>
>
> Repository: geode
>
>
> Description
> -------
>
> It's both GEODE-920, GEM-454, GEM-525.
>
> If gateway receiver is created before cache server, the notify subscriptions attributes can never been set, since CCN singleton is created by the gateway receiver.
>
> The easy fix is not to let gateway receiver create the CCN singleton.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/InitialImageOperation.java 9bd3faf
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java 8e30a7a
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ha/HAContainerRegion.java 3bed769
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/CacheClientNotifier.java 6ac4690
> geode-wan/src/test/java/com/gemstone/gemfire/internal/cache/wan/CacheClientNotifierDUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45336/diff/
>
>
> Testing
> -------
>
> - introduced new dunit test.
> - precheckin
> - regression tests:
>
>
> Thanks,
>
> xiaojian zhou
>
>
Re: Review Request 45336: GEODE-920: Not to create CCN for gateway
receiver
Posted by xiaojian zhou <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45336/
-----------------------------------------------------------
(Updated April 1, 2016, 10:11 p.m.)
Review request for geode, anilkumar gingade and Dan Smith.
Changes
-------
fix based on anil's comments
Bugs: GEM-454, GEM-525 and GEODE-920
https://issues.apache.org/jira/browse/GEM-454
https://issues.apache.org/jira/browse/GEM-525
https://issues.apache.org/jira/browse/GEODE-920
Repository: geode
Description
-------
It's both GEODE-920, GEM-454, GEM-525.
If gateway receiver is created before cache server, the notify subscriptions attributes can never been set, since CCN singleton is created by the gateway receiver.
The easy fix is not to let gateway receiver create the CCN singleton.
Diffs (updated)
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/InitialImageOperation.java 9bd3faf
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java 8e30a7a
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ha/HAContainerRegion.java 3bed769
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/CacheClientNotifier.java 6ac4690
geode-wan/src/test/java/com/gemstone/gemfire/internal/cache/wan/CacheClientNotifierDUnitTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/45336/diff/
Testing
-------
- introduced new dunit test.
- precheckin
- regression tests:
Thanks,
xiaojian zhou
Re: Review Request 45336: GEODE-920: Not to create CCN for gateway
receiver
Posted by xiaojian zhou <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45336/
-----------------------------------------------------------
(Updated April 1, 2016, 12:37 a.m.)
Review request for geode, anilkumar gingade and Dan Smith.
Changes
-------
This is a new fix based on a new idea.
The CCN will be created by even the gateway receiver. However, the haContainer will be kept null and wait for real cache server to initialize it.
Bugs: GEM-454, GEM-525 and GEODE-920
https://issues.apache.org/jira/browse/GEM-454
https://issues.apache.org/jira/browse/GEM-525
https://issues.apache.org/jira/browse/GEODE-920
Repository: geode
Description
-------
It's both GEODE-920, GEM-454, GEM-525.
If gateway receiver is created before cache server, the notify subscriptions attributes can never been set, since CCN singleton is created by the gateway receiver.
The easy fix is not to let gateway receiver create the CCN singleton.
Diffs (updated)
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/InitialImageOperation.java 9bd3faf
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java 95616fe
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ha/HAContainerRegion.java 3bed769
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/AcceptorImpl.java 5b20e86
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/CacheClientNotifier.java 6ac4690
geode-wan/src/test/java/com/gemstone/gemfire/internal/cache/wan/CacheClientNotifierDUnitTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/45336/diff/
Testing
-------
- introduced new dunit test.
- precheckin
- regression tests:
Thanks,
xiaojian zhou
Re: Review Request 45336: GEODE-920: Not to create CCN for gateway
receiver
Posted by anilkumar gingade <ag...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45336/#review125450
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/InitialImageOperation.java (line 2184)
<https://reviews.apache.org/r/45336/#comment188234>
This is under debug logic...You may need to log it as log.debug()...
And change message to reflect the action "Processing FilterInfo"...
You don't want to reurn from here...This needs to fall through to get other part of the code to get executed, Similar to error handlling in this logic...
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/InitialImageOperation.java (line 3717)
<https://reviews.apache.org/r/45336/#comment188233>
Can we change the log message to reflect the action...Similar to following messge in this method:
logger.info("Found null client proxy. Failed to register Filters during HARegion GII. Region :{}", region.getName());
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/AcceptorImpl.java (line 534)
<https://reviews.apache.org/r/45336/#comment188236>
This comment confuses...Can we change this to.
//Fix for GEM-525, CacheClientNotifier is created for subscription cache servers, not for Gateway Reciever.
geode-wan/src/test/java/com/gemstone/gemfire/internal/cache/wan/CacheClientNotifierDUnitTest.java (line 181)
<https://reviews.apache.org/r/45336/#comment188237>
How about checking for CCN here; it should not have created....
- anilkumar gingade
On March 25, 2016, 5:59 p.m., xiaojian zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45336/
> -----------------------------------------------------------
>
> (Updated March 25, 2016, 5:59 p.m.)
>
>
> Review request for geode, anilkumar gingade and Dan Smith.
>
>
> Bugs: GEM-454, GEM-525 and GEODE-920
> https://issues.apache.org/jira/browse/GEM-454
> https://issues.apache.org/jira/browse/GEM-525
> https://issues.apache.org/jira/browse/GEODE-920
>
>
> Repository: geode
>
>
> Description
> -------
>
> It's both GEODE-920, GEM-454, GEM-525.
>
> If gateway receiver is created before cache server, the notify subscriptions attributes can never been set, since CCN singleton is created by the gateway receiver.
>
> The easy fix is not to let gateway receiver create the CCN singleton.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/InitialImageOperation.java 34e3855
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ha/HAContainerRegion.java 8803c32
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/AcceptorImpl.java 9f18f50
> geode-wan/src/test/java/com/gemstone/gemfire/internal/cache/wan/CacheClientNotifierDUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45336/diff/
>
>
> Testing
> -------
>
> - introduced new dunit test.
> - precheckin
> - regression tests:
>
>
> Thanks,
>
> xiaojian zhou
>
>
Re: Review Request 45336: GEODE-920: Not to create CCN for gateway
receiver
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45336/#review125443
-----------------------------------------------------------
Fix it, then Ship it!
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/AcceptorImpl.java (line 543)
<https://reviews.apache.org/r/45336/#comment188230>
Why is this using a dummy statistics factory?
geode-wan/src/test/java/com/gemstone/gemfire/internal/cache/wan/CacheClientNotifierDUnitTest.java (line 165)
<https://reviews.apache.org/r/45336/#comment188231>
spelling of CacheServerr
- Dan Smith
On March 25, 2016, 5:59 p.m., xiaojian zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45336/
> -----------------------------------------------------------
>
> (Updated March 25, 2016, 5:59 p.m.)
>
>
> Review request for geode, anilkumar gingade and Dan Smith.
>
>
> Bugs: GEM-454, GEM-525 and GEODE-920
> https://issues.apache.org/jira/browse/GEM-454
> https://issues.apache.org/jira/browse/GEM-525
> https://issues.apache.org/jira/browse/GEODE-920
>
>
> Repository: geode
>
>
> Description
> -------
>
> It's both GEODE-920, GEM-454, GEM-525.
>
> If gateway receiver is created before cache server, the notify subscriptions attributes can never been set, since CCN singleton is created by the gateway receiver.
>
> The easy fix is not to let gateway receiver create the CCN singleton.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/InitialImageOperation.java 34e3855
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ha/HAContainerRegion.java 8803c32
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/AcceptorImpl.java 9f18f50
> geode-wan/src/test/java/com/gemstone/gemfire/internal/cache/wan/CacheClientNotifierDUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45336/diff/
>
>
> Testing
> -------
>
> - introduced new dunit test.
> - precheckin
> - regression tests:
>
>
> Thanks,
>
> xiaojian zhou
>
>
Re: Review Request 45336: GEODE-920: Not to create CCN for gateway
receiver
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45336/#review125466
-----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/AcceptorImpl.java (line 534)
<https://reviews.apache.org/r/45336/#comment188261>
Don't put pivotal bug numbers into apache code.
geode-wan/src/test/java/com/gemstone/gemfire/internal/cache/wan/CacheClientNotifierDUnitTest.java (line 170)
<https://reviews.apache.org/r/45336/#comment188260>
Another pivotal bug number.
- Dan Smith
On March 25, 2016, 5:59 p.m., xiaojian zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45336/
> -----------------------------------------------------------
>
> (Updated March 25, 2016, 5:59 p.m.)
>
>
> Review request for geode, anilkumar gingade and Dan Smith.
>
>
> Bugs: GEM-454, GEM-525 and GEODE-920
> https://issues.apache.org/jira/browse/GEM-454
> https://issues.apache.org/jira/browse/GEM-525
> https://issues.apache.org/jira/browse/GEODE-920
>
>
> Repository: geode
>
>
> Description
> -------
>
> It's both GEODE-920, GEM-454, GEM-525.
>
> If gateway receiver is created before cache server, the notify subscriptions attributes can never been set, since CCN singleton is created by the gateway receiver.
>
> The easy fix is not to let gateway receiver create the CCN singleton.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/InitialImageOperation.java 34e3855
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/ha/HAContainerRegion.java 8803c32
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/AcceptorImpl.java 9f18f50
> geode-wan/src/test/java/com/gemstone/gemfire/internal/cache/wan/CacheClientNotifierDUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45336/diff/
>
>
> Testing
> -------
>
> - introduced new dunit test.
> - precheckin
> - regression tests:
>
>
> Thanks,
>
> xiaojian zhou
>
>