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 18:41:00 UTC
Review Request 53094: GEODE-706 race condition between expiry thread
and other user thread.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53094/
-----------------------------------------------------------
Review request for geode, Bruce Schuchardt, Darrel Schneider, and Udo Kohlmeyer.
Repository: geode
Description
-------
ExpirationManager tracks the regionEntry as reference to expiryTask. It assumes, it is unique for that key. But consistency check mechanism keeps that regionEntry around and that enforces re-use of that for new update. That introduces the race condition between expiry thread and user thread, it endup not scheduling the entry for expiration. As a fix, now "consistency check mechanism" unschedules the expiry task to avoid this race condition.
Diffs
-----
geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java e02c7e1
geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java d059aab
geode-core/src/main/java/org/apache/geode/internal/cache/EntryExpiryTask.java 0c20d32
geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java ac4c705
Diff: https://reviews.apache.org/r/53094/diff/
Testing
-------
Thanks,
Hitesh Khamesra
Re: Review Request 53094: GEODE-706 race condition between expiry
thread and other user thread.
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53094/#review153596
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java (line 8660)
<https://reviews.apache.org/r/53094/#comment223003>
It would be better to remove these calls instead of just commenting them out.
- Darrel Schneider
On Oct. 21, 2016, 11:41 a.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53094/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2016, 11:41 a.m.)
>
>
> Review request for geode, Bruce Schuchardt, Darrel Schneider, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> ExpirationManager tracks the regionEntry as reference to expiryTask. It assumes, it is unique for that key. But consistency check mechanism keeps that regionEntry around and that enforces re-use of that for new update. That introduces the race condition between expiry thread and user thread, it endup not scheduling the entry for expiration. As a fix, now "consistency check mechanism" unschedules the expiry task to avoid this race condition.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java e02c7e1
> geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java d059aab
> geode-core/src/main/java/org/apache/geode/internal/cache/EntryExpiryTask.java 0c20d32
> geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java ac4c705
>
> Diff: https://reviews.apache.org/r/53094/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 53094: GEODE-706 race condition between expiry
thread and other user thread.
Posted by Bruce Schuchardt <bs...@pivotal.io>.
That's a better comment. You should replace the one you have with it.
Le 10/21/2016 � 4:03 PM, Hitesh Khamesra a �crit :
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53094/
>
>
> On October 21st, 2016, 10:36 p.m. UTC, *Bruce Schuchardt* wrote:
>
> geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
> <https://reviews.apache.org/r/53094/diff/1/?file=1543182#file1543182line1516>
> (Diff revision 1)
>
> public final boolean destroy(EntryEventImpl event,
>
> 1516
>
> owner.cancelExpiryTask(re, event.getExpiryTask());
>
> 1516
>
> //we only want to cancel if concurrency-check is not enabled
>
> I don't understand this comment. There doesn't seem to be a
> check for concurrency-checks being enabled & you've removed
> the check for a tombstone.
>
> Right, I just added some more comment there. Basically regionEntry
> will be null if concurrency-check is enable. And in that case we
> cancel expiry from removeTomstone method.
>
>
> //we only want to cancel if concurrency-check is not enabled
> //re(regionentry) will be null when concurrency-check is enable and
> removeTombstone method //will call cancelExpiryTask on regionEntry
>
>
> - Hitesh
>
>
> On October 21st, 2016, 6:41 p.m. UTC, Hitesh Khamesra wrote:
>
> Review request for geode, Bruce Schuchardt, Darrel Schneider, and Udo
> Kohlmeyer.
> By Hitesh Khamesra.
>
> /Updated Oct. 21, 2016, 6:41 p.m./
>
> *Repository: * geode
>
>
> Description
>
> ExpirationManager tracks the regionEntry as reference to expiryTask.
> It assumes, it is unique for that key. But consistency check mechanism
> keeps that regionEntry around and that enforces re-use of that for new
> update. That introduces the race condition between expiry thread and
> user thread, it endup not scheduling the entry for expiration. As a
> fix, now "consistency check mechanism" unschedules the expiry task to
> avoid this race condition.
>
>
> Diffs
>
> * geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
> (e02c7e1)
> * geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
> (d059aab)
> * geode-core/src/main/java/org/apache/geode/internal/cache/EntryExpiryTask.java
> (0c20d32)
> * geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
> (ac4c705)
>
> View Diff <https://reviews.apache.org/r/53094/diff/>
>
Re: Review Request 53094: GEODE-706 race condition between expiry
thread and other user thread.
Posted by Hitesh Khamesra <hk...@pivotal.io>.
> On Oct. 21, 2016, 10:36 p.m., Bruce Schuchardt wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java, line 1516
> > <https://reviews.apache.org/r/53094/diff/1/?file=1543182#file1543182line1516>
> >
> > I don't understand this comment. There doesn't seem to be a check for concurrency-checks being enabled & you've removed the check for a tombstone.
Right, I just added some more comment there. Basically regionEntry will be null if concurrency-check is enable. And in that case we cancel expiry from removeTomstone method.
//we only want to cancel if concurrency-check is not enabled
//re(regionentry) will be null when concurrency-check is enable and removeTombstone method
//will call cancelExpiryTask on regionEntry
- Hitesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53094/#review153607
-----------------------------------------------------------
On Oct. 21, 2016, 6:41 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53094/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2016, 6:41 p.m.)
>
>
> Review request for geode, Bruce Schuchardt, Darrel Schneider, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> ExpirationManager tracks the regionEntry as reference to expiryTask. It assumes, it is unique for that key. But consistency check mechanism keeps that regionEntry around and that enforces re-use of that for new update. That introduces the race condition between expiry thread and user thread, it endup not scheduling the entry for expiration. As a fix, now "consistency check mechanism" unschedules the expiry task to avoid this race condition.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java e02c7e1
> geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java d059aab
> geode-core/src/main/java/org/apache/geode/internal/cache/EntryExpiryTask.java 0c20d32
> geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java ac4c705
>
> Diff: https://reviews.apache.org/r/53094/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 53094: GEODE-706 race condition between expiry
thread and other user thread.
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53094/#review153607
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java (line 1516)
<https://reviews.apache.org/r/53094/#comment223014>
I don't understand this comment. There doesn't seem to be a check for concurrency-checks being enabled & you've removed the check for a tombstone.
- Bruce Schuchardt
On Oct. 21, 2016, 6:41 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53094/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2016, 6:41 p.m.)
>
>
> Review request for geode, Bruce Schuchardt, Darrel Schneider, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> ExpirationManager tracks the regionEntry as reference to expiryTask. It assumes, it is unique for that key. But consistency check mechanism keeps that regionEntry around and that enforces re-use of that for new update. That introduces the race condition between expiry thread and user thread, it endup not scheduling the entry for expiration. As a fix, now "consistency check mechanism" unschedules the expiry task to avoid this race condition.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java e02c7e1
> geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java d059aab
> geode-core/src/main/java/org/apache/geode/internal/cache/EntryExpiryTask.java 0c20d32
> geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java ac4c705
>
> Diff: https://reviews.apache.org/r/53094/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>