You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by anilkumar gingade <ag...@pivotal.io> on 2016/06/03 00:49:19 UTC
Review Request 48188: GEODE-1495: Changes are made to remove the
cached destroyed token/events from the CQ.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48188/
-----------------------------------------------------------
Review request for geode, anilkumar gingade, Barry Oglesby, Bruce Schuchardt, Jason Huynh, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
Repository: geode
Description
-------
The CQEvents as seen by CQs are cached in order to avoid applying CQ queries on old values.
In case of a destory CQEvent, the CQEvents are marked with destroy tokens and removed from
the cache after the CQEvent is added to HAQueue.
This works fine for the CQs registered locally, but for the CQs registered on peer server, the
CQs weren't removed from the cache, which resulted in generating wrong CQEvent for subsequent
operation.
This change removes the destroy CQevent from the cache after the CQEvent is distributed to
peer server.
Diffs
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DistributedCacheOperation.java 6a7b4f2
Diff: https://reviews.apache.org/r/48188/diff/
Testing
-------
Reproduce the issue with manual testing. The test passed after the changes are made to remove cached destroy events from remote CQs.
Thanks,
anilkumar gingade
Re: Review Request 48188: GEODE-1495: Changes are made to remove the
cached destroyed token/events from the CQ.
Posted by anilkumar gingade <ag...@pivotal.io>.
> On June 3, 2016, 10:10 p.m., Dan Smith wrote:
> > I think there should be a test with these changes.
Dan, I am adding that...Wanted to get early feedback on this and get tests started...
I will add the test changes to this review post.
- anilkumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48188/#review136116
-----------------------------------------------------------
On June 3, 2016, 12:49 a.m., anilkumar gingade wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48188/
> -----------------------------------------------------------
>
> (Updated June 3, 2016, 12:49 a.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, Bruce Schuchardt, Jason Huynh, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> The CQEvents as seen by CQs are cached in order to avoid applying CQ queries on old values.
>
> In case of a destory CQEvent, the CQEvents are marked with destroy tokens and removed from
> the cache after the CQEvent is added to HAQueue.
> This works fine for the CQs registered locally, but for the CQs registered on peer server, the
> CQs weren't removed from the cache, which resulted in generating wrong CQEvent for subsequent
> operation.
> This change removes the destroy CQevent from the cache after the CQEvent is distributed to
> peer server.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DistributedCacheOperation.java 6a7b4f2
>
> Diff: https://reviews.apache.org/r/48188/diff/
>
>
> Testing
> -------
>
> Reproduce the issue with manual testing. The test passed after the changes are made to remove cached destroy events from remote CQs.
>
>
> Thanks,
>
> anilkumar gingade
>
>
Re: Review Request 48188: GEODE-1495: Changes are made to remove the
cached destroyed token/events from the CQ.
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48188/#review136116
-----------------------------------------------------------
I think there should be a test with these changes.
- Dan Smith
On June 3, 2016, 12:49 a.m., anilkumar gingade wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48188/
> -----------------------------------------------------------
>
> (Updated June 3, 2016, 12:49 a.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, Bruce Schuchardt, Jason Huynh, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> The CQEvents as seen by CQs are cached in order to avoid applying CQ queries on old values.
>
> In case of a destory CQEvent, the CQEvents are marked with destroy tokens and removed from
> the cache after the CQEvent is added to HAQueue.
> This works fine for the CQs registered locally, but for the CQs registered on peer server, the
> CQs weren't removed from the cache, which resulted in generating wrong CQEvent for subsequent
> operation.
> This change removes the destroy CQevent from the cache after the CQEvent is distributed to
> peer server.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DistributedCacheOperation.java 6a7b4f2
>
> Diff: https://reviews.apache.org/r/48188/diff/
>
>
> Testing
> -------
>
> Reproduce the issue with manual testing. The test passed after the changes are made to remove cached destroy events from remote CQs.
>
>
> Thanks,
>
> anilkumar gingade
>
>
Re: Review Request 48188: GEODE-1495: Changes are made to remove the
cached destroyed token/events from the CQ.
Posted by Dan Smith <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48188/#review136389
-----------------------------------------------------------
Ship it!
Ship It!
- Dan Smith
On June 6, 2016, 8:26 p.m., anilkumar gingade wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48188/
> -----------------------------------------------------------
>
> (Updated June 6, 2016, 8:26 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, Bruce Schuchardt, Jason Huynh, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> The CQEvents as seen by CQs are cached in order to avoid applying CQ queries on old values.
>
> In case of a destory CQEvent, the CQEvents are marked with destroy tokens and removed from
> the cache after the CQEvent is added to HAQueue.
> This works fine for the CQs registered locally, but for the CQs registered on peer server, the
> CQs weren't removed from the cache, which resulted in generating wrong CQEvent for subsequent
> operation.
> This change removes the destroy CQevent from the cache after the CQEvent is distributed to
> peer server.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DistributedCacheOperation.java 6a7b4f2
> geode-cq/src/test/java/com/gemstone/gemfire/cache/query/cq/dunit/PrCqUsingPoolDUnitTest.java ee8ea9f
>
> Diff: https://reviews.apache.org/r/48188/diff/
>
>
> Testing
> -------
>
> Reproduce the issue with manual testing. The test passed after the changes are made to remove cached destroy events from remote CQs.
>
>
> Thanks,
>
> anilkumar gingade
>
>
Re: Review Request 48188: GEODE-1495: Changes are made to remove the
cached destroyed token/events from the CQ.
Posted by anilkumar gingade <ag...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48188/
-----------------------------------------------------------
(Updated June 6, 2016, 8:26 p.m.)
Review request for geode, anilkumar gingade, Barry Oglesby, Bruce Schuchardt, Jason Huynh, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
Changes
-------
Addressed NPE with CacheProfile (cf.filterProfile) found during testing.
Repository: geode
Description
-------
The CQEvents as seen by CQs are cached in order to avoid applying CQ queries on old values.
In case of a destory CQEvent, the CQEvents are marked with destroy tokens and removed from
the cache after the CQEvent is added to HAQueue.
This works fine for the CQs registered locally, but for the CQs registered on peer server, the
CQs weren't removed from the cache, which resulted in generating wrong CQEvent for subsequent
operation.
This change removes the destroy CQevent from the cache after the CQEvent is distributed to
peer server.
Diffs (updated)
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DistributedCacheOperation.java 6a7b4f2
geode-cq/src/test/java/com/gemstone/gemfire/cache/query/cq/dunit/PrCqUsingPoolDUnitTest.java ee8ea9f
Diff: https://reviews.apache.org/r/48188/diff/
Testing
-------
Reproduce the issue with manual testing. The test passed after the changes are made to remove cached destroy events from remote CQs.
Thanks,
anilkumar gingade
Re: Review Request 48188: GEODE-1495: Changes are made to remove the
cached destroyed token/events from the CQ.
Posted by anilkumar gingade <ag...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48188/
-----------------------------------------------------------
(Updated June 4, 2016, 1:20 a.m.)
Review request for geode, anilkumar gingade, Barry Oglesby, Bruce Schuchardt, Jason Huynh, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
Changes
-------
Added Unit test. Added missing braces for if conditions...
Repository: geode
Description
-------
The CQEvents as seen by CQs are cached in order to avoid applying CQ queries on old values.
In case of a destory CQEvent, the CQEvents are marked with destroy tokens and removed from
the cache after the CQEvent is added to HAQueue.
This works fine for the CQs registered locally, but for the CQs registered on peer server, the
CQs weren't removed from the cache, which resulted in generating wrong CQEvent for subsequent
operation.
This change removes the destroy CQevent from the cache after the CQEvent is distributed to
peer server.
Diffs (updated)
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DistributedCacheOperation.java 6a7b4f2
geode-cq/src/test/java/com/gemstone/gemfire/cache/query/cq/dunit/PrCqUsingPoolDUnitTest.java ee8ea9f
Diff: https://reviews.apache.org/r/48188/diff/
Testing
-------
Reproduce the issue with manual testing. The test passed after the changes are made to remove cached destroy events from remote CQs.
Thanks,
anilkumar gingade
Re: Review Request 48188: GEODE-1495: Changes are made to remove the
cached destroyed token/events from the CQ.
Posted by anilkumar gingade <ag...@pivotal.io>.
> On June 3, 2016, 10:05 p.m., Bruce Schuchardt wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DistributedCacheOperation.java, line 696
> > <https://reviews.apache.org/r/48188/diff/1/?file=1405413#file1405413line696>
> >
> > You're missing braces on all of your two-line "if" statements.
Will add the comments.
> On June 3, 2016, 10:05 p.m., Bruce Schuchardt wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DistributedCacheOperation.java, line 693
> > <https://reviews.apache.org/r/48188/diff/1/?file=1405413#file1405413line693>
> >
> > I wonder if there isn't a better home for this method than DistributedCacheOperation?
I felt the same way; but come across the right place to add this...The logic needed to be added after the event is processed and sent across...
Some of the option i thought:
1. Using CacheClientNotifier where this is done for local CQs. But in this case (peer node) there may not be CacheClientNotifier...
- anilkumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48188/#review136115
-----------------------------------------------------------
On June 3, 2016, 12:49 a.m., anilkumar gingade wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48188/
> -----------------------------------------------------------
>
> (Updated June 3, 2016, 12:49 a.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, Bruce Schuchardt, Jason Huynh, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> The CQEvents as seen by CQs are cached in order to avoid applying CQ queries on old values.
>
> In case of a destory CQEvent, the CQEvents are marked with destroy tokens and removed from
> the cache after the CQEvent is added to HAQueue.
> This works fine for the CQs registered locally, but for the CQs registered on peer server, the
> CQs weren't removed from the cache, which resulted in generating wrong CQEvent for subsequent
> operation.
> This change removes the destroy CQevent from the cache after the CQEvent is distributed to
> peer server.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DistributedCacheOperation.java 6a7b4f2
>
> Diff: https://reviews.apache.org/r/48188/diff/
>
>
> Testing
> -------
>
> Reproduce the issue with manual testing. The test passed after the changes are made to remove cached destroy events from remote CQs.
>
>
> Thanks,
>
> anilkumar gingade
>
>
Re: Review Request 48188: GEODE-1495: Changes are made to remove the
cached destroyed token/events from the CQ.
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48188/#review136115
-----------------------------------------------------------
Fix it, then Ship it!
Fix it, then Ship it!
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DistributedCacheOperation.java (line 693)
<https://reviews.apache.org/r/48188/#comment201125>
I wonder if there isn't a better home for this method than DistributedCacheOperation?
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DistributedCacheOperation.java (line 696)
<https://reviews.apache.org/r/48188/#comment201124>
You're missing braces on all of your two-line "if" statements.
- Bruce Schuchardt
On June 3, 2016, 12:49 a.m., anilkumar gingade wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48188/
> -----------------------------------------------------------
>
> (Updated June 3, 2016, 12:49 a.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, Bruce Schuchardt, Jason Huynh, William Markito, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> The CQEvents as seen by CQs are cached in order to avoid applying CQ queries on old values.
>
> In case of a destory CQEvent, the CQEvents are marked with destroy tokens and removed from
> the cache after the CQEvent is added to HAQueue.
> This works fine for the CQs registered locally, but for the CQs registered on peer server, the
> CQs weren't removed from the cache, which resulted in generating wrong CQEvent for subsequent
> operation.
> This change removes the destroy CQevent from the cache after the CQEvent is distributed to
> peer server.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/DistributedCacheOperation.java 6a7b4f2
>
> Diff: https://reviews.apache.org/r/48188/diff/
>
>
> Testing
> -------
>
> Reproduce the issue with manual testing. The test passed after the changes are made to remove cached destroy events from remote CQs.
>
>
> Thanks,
>
> anilkumar gingade
>
>