You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Ken Howe <kh...@pivotal.io> on 2016/10/14 22:39:41 UTC
Review Request 52891: GEODE-538: Add check for persistent data
recovery
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52891/
-----------------------------------------------------------
Review request for geode, anilkumar gingade, Darrel Schneider, Eric Shu, Scott Jewell, and Dan Smith.
Repository: geode
Description
-------
PartitionedRegion.getNodeForBucketReadOrLoad can return an invalid node
if persistent data recovery is in process and a get() targets a bucket
that
hasn't been recoverd yet. This can result in returning an incorrect
value (null) or throwing ConflictingPersistentDataException from a get()
or put() on the region.
This change adds a check for persistent recovery to be completed
before creating the new bucket. If recovery isn't complete then the
operation on the region will fail with a PartitionOfflineException.
Queries on a region while persistent recovery is in progress can also
result in incorrect results so a similar check is added to
DefaultQuery.checkQueryOnPR.
New DUnit tests added for gets, puts, and queries for cases where persistent
colocated child regions haven't been started and where the child region
has been started but persistent recovery is still in progress when
the region operation is attempted.
Diffs
-----
geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQuery.java 58df3904e32b1af140bda92e0aba16da28c6a109
geode-core/src/main/java/org/apache/geode/internal/cache/PRHARedundancyProvider.java cfedb677bceb884cd726f26e5bd0047876a668ab
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java baab79f930ab0eca03ab04660a21d10f5508f578
geode-core/src/main/java/org/apache/geode/internal/i18n/LocalizedStrings.java 8bfdd686eb50605e12cb59015d5ddab99714c563
geode-core/src/test/java/org/apache/geode/cache/query/partitioned/PRBasicQueryDUnitTest.java 8ef907ac85195d0433f3e2f923a7a9e02fc48fff
geode-core/src/test/java/org/apache/geode/cache/query/partitioned/PRQueryDUnitHelper.java cfb419036817d994ef49e4e0bcfb304d15db300e
geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java 0a25228c9b2fd5066bbaf7f806462bcff36f0ba9
Diff: https://reviews.apache.org/r/52891/diff/
Testing
-------
precheckin
Thanks,
Ken Howe
Re: Review Request 52891: GEODE-538: Add check for persistent data
recovery
Posted by Ken Howe <kh...@pivotal.io>.
> On Oct. 17, 2016, 7:49 p.m., anilkumar gingade wrote:
> >
Added the @throws PartitionOfflineException to the javadocs for the other (non-0-argument) execute methods.
> On Oct. 17, 2016, 7:49 p.m., anilkumar gingade wrote:
> > geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQuery.java, line 608
> > <https://reviews.apache.org/r/52891/diff/1/?file=1538164#file1538164line608>
> >
> > We are caling this in two places...And we are trying to determine offline disk stores while throwing exception...Can we move this to a method in PR...And call that function here...
> > E.g.:
> >
> > isPROffline() throws PartitionOfflineException () {
> > }
Removed the call of getDistributionAdvisor() here and in PRHARedundancyProvider - it's not needed (was left over from an earlier implementation of the fix.)
Moved the check to a new method in PartitionedRegion as suggested.
- Ken
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52891/#review152936
-----------------------------------------------------------
On Oct. 17, 2016, 11:15 p.m., Ken Howe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52891/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2016, 11:15 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Eric Shu, Scott Jewell, and Dan Smith.
>
>
> Repository: geode
>
>
> Description
> -------
>
> PartitionedRegion.getNodeForBucketReadOrLoad can return an invalid node
> if persistent data recovery is in process and a get() targets a bucket
> that
> hasn't been recoverd yet. This can result in returning an incorrect
> value (null) or throwing ConflictingPersistentDataException from a get()
> or put() on the region.
>
> This change adds a check for persistent recovery to be completed
> before creating the new bucket. If recovery isn't complete then the
> operation on the region will fail with a PartitionOfflineException.
>
> Queries on a region while persistent recovery is in progress can also
> result in incorrect results so a similar check is added to
> DefaultQuery.checkQueryOnPR.
>
> New DUnit tests added for gets, puts, and queries for cases where persistent
> colocated child regions haven't been started and where the child region
> has been started but persistent recovery is still in progress when
> the region operation is attempted.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/cache/query/Query.java e27687d
> geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQuery.java 58df390
> geode-core/src/main/java/org/apache/geode/internal/cache/PRHARedundancyProvider.java cfedb67
> geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java baab79f
> geode-core/src/main/java/org/apache/geode/internal/i18n/LocalizedStrings.java 8bfdd68
> geode-core/src/test/java/org/apache/geode/cache/query/partitioned/PRBasicQueryDUnitTest.java 8ef907a
> geode-core/src/test/java/org/apache/geode/cache/query/partitioned/PRQueryDUnitHelper.java cfb4190
> geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java 0a25228
>
> Diff: https://reviews.apache.org/r/52891/diff/
>
>
> Testing
> -------
>
> precheckin
>
>
> Thanks,
>
> Ken Howe
>
>
Re: Review Request 52891: GEODE-538: Add check for persistent data
recovery
Posted by anilkumar gingade <ag...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52891/#review152936
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQuery.java (line 608)
<https://reviews.apache.org/r/52891/#comment222128>
We are caling this in two places...And we are trying to determine offline disk stores while throwing exception...Can we move this to a method in PR...And call that function here...
E.g.:
isPROffline() throws PartitionOfflineException () {
}
geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQuery.java (line 611)
<https://reviews.apache.org/r/52891/#comment222127>
We need to add this exception to the javadoc for query.execute(*) (Query interface).
- anilkumar gingade
On Oct. 14, 2016, 10:39 p.m., Ken Howe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52891/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2016, 10:39 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Eric Shu, Scott Jewell, and Dan Smith.
>
>
> Repository: geode
>
>
> Description
> -------
>
> PartitionedRegion.getNodeForBucketReadOrLoad can return an invalid node
> if persistent data recovery is in process and a get() targets a bucket
> that
> hasn't been recoverd yet. This can result in returning an incorrect
> value (null) or throwing ConflictingPersistentDataException from a get()
> or put() on the region.
>
> This change adds a check for persistent recovery to be completed
> before creating the new bucket. If recovery isn't complete then the
> operation on the region will fail with a PartitionOfflineException.
>
> Queries on a region while persistent recovery is in progress can also
> result in incorrect results so a similar check is added to
> DefaultQuery.checkQueryOnPR.
>
> New DUnit tests added for gets, puts, and queries for cases where persistent
> colocated child regions haven't been started and where the child region
> has been started but persistent recovery is still in progress when
> the region operation is attempted.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQuery.java 58df3904e32b1af140bda92e0aba16da28c6a109
> geode-core/src/main/java/org/apache/geode/internal/cache/PRHARedundancyProvider.java cfedb677bceb884cd726f26e5bd0047876a668ab
> geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java baab79f930ab0eca03ab04660a21d10f5508f578
> geode-core/src/main/java/org/apache/geode/internal/i18n/LocalizedStrings.java 8bfdd686eb50605e12cb59015d5ddab99714c563
> geode-core/src/test/java/org/apache/geode/cache/query/partitioned/PRBasicQueryDUnitTest.java 8ef907ac85195d0433f3e2f923a7a9e02fc48fff
> geode-core/src/test/java/org/apache/geode/cache/query/partitioned/PRQueryDUnitHelper.java cfb419036817d994ef49e4e0bcfb304d15db300e
> geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java 0a25228c9b2fd5066bbaf7f806462bcff36f0ba9
>
> Diff: https://reviews.apache.org/r/52891/diff/
>
>
> Testing
> -------
>
> precheckin
>
>
> Thanks,
>
> Ken Howe
>
>
Re: Review Request 52891: GEODE-538: Add check for persistent data
recovery
Posted by Ken Howe <kh...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52891/
-----------------------------------------------------------
(Updated Oct. 18, 2016, 8:25 p.m.)
Review request for geode, anilkumar gingade, Darrel Schneider, Eric Shu, Scott Jewell, and Dan Smith.
Changes
-------
Added to @throws to the other Query.execute methods
Repository: geode
Description
-------
PartitionedRegion.getNodeForBucketReadOrLoad can return an invalid node
if persistent data recovery is in process and a get() targets a bucket
that
hasn't been recoverd yet. This can result in returning an incorrect
value (null) or throwing ConflictingPersistentDataException from a get()
or put() on the region.
This change adds a check for persistent recovery to be completed
before creating the new bucket. If recovery isn't complete then the
operation on the region will fail with a PartitionOfflineException.
Queries on a region while persistent recovery is in progress can also
result in incorrect results so a similar check is added to
DefaultQuery.checkQueryOnPR.
New DUnit tests added for gets, puts, and queries for cases where persistent
colocated child regions haven't been started and where the child region
has been started but persistent recovery is still in progress when
the region operation is attempted.
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/cache/query/Query.java e27687d
geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQuery.java 58df390
geode-core/src/main/java/org/apache/geode/internal/cache/PRHARedundancyProvider.java cfedb67
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java baab79f
geode-core/src/main/java/org/apache/geode/internal/i18n/LocalizedStrings.java 8bfdd68
geode-core/src/test/java/org/apache/geode/cache/query/partitioned/PRBasicQueryDUnitTest.java 8ef907a
geode-core/src/test/java/org/apache/geode/cache/query/partitioned/PRQueryDUnitHelper.java cfb4190
geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java 0a25228
Diff: https://reviews.apache.org/r/52891/diff/
Testing
-------
precheckin
Thanks,
Ken Howe
Re: Review Request 52891: GEODE-538: Add check for persistent data
recovery
Posted by anilkumar gingade <ag...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52891/#review153129
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
geode-core/src/main/java/org/apache/geode/cache/query/Query.java (line 92)
<https://reviews.apache.org/r/52891/#comment222410>
Wemay have to add this to other flavors of execute(*)...
- anilkumar gingade
On Oct. 17, 2016, 11:15 p.m., Ken Howe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52891/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2016, 11:15 p.m.)
>
>
> Review request for geode, anilkumar gingade, Darrel Schneider, Eric Shu, Scott Jewell, and Dan Smith.
>
>
> Repository: geode
>
>
> Description
> -------
>
> PartitionedRegion.getNodeForBucketReadOrLoad can return an invalid node
> if persistent data recovery is in process and a get() targets a bucket
> that
> hasn't been recoverd yet. This can result in returning an incorrect
> value (null) or throwing ConflictingPersistentDataException from a get()
> or put() on the region.
>
> This change adds a check for persistent recovery to be completed
> before creating the new bucket. If recovery isn't complete then the
> operation on the region will fail with a PartitionOfflineException.
>
> Queries on a region while persistent recovery is in progress can also
> result in incorrect results so a similar check is added to
> DefaultQuery.checkQueryOnPR.
>
> New DUnit tests added for gets, puts, and queries for cases where persistent
> colocated child regions haven't been started and where the child region
> has been started but persistent recovery is still in progress when
> the region operation is attempted.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/cache/query/Query.java e27687d
> geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQuery.java 58df390
> geode-core/src/main/java/org/apache/geode/internal/cache/PRHARedundancyProvider.java cfedb67
> geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java baab79f
> geode-core/src/main/java/org/apache/geode/internal/i18n/LocalizedStrings.java 8bfdd68
> geode-core/src/test/java/org/apache/geode/cache/query/partitioned/PRBasicQueryDUnitTest.java 8ef907a
> geode-core/src/test/java/org/apache/geode/cache/query/partitioned/PRQueryDUnitHelper.java cfb4190
> geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java 0a25228
>
> Diff: https://reviews.apache.org/r/52891/diff/
>
>
> Testing
> -------
>
> precheckin
>
>
> Thanks,
>
> Ken Howe
>
>
Re: Review Request 52891: GEODE-538: Add check for persistent data
recovery
Posted by Ken Howe <kh...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52891/
-----------------------------------------------------------
(Updated Oct. 17, 2016, 11:15 p.m.)
Review request for geode, anilkumar gingade, Darrel Schneider, Eric Shu, Scott Jewell, and Dan Smith.
Repository: geode
Description
-------
PartitionedRegion.getNodeForBucketReadOrLoad can return an invalid node
if persistent data recovery is in process and a get() targets a bucket
that
hasn't been recoverd yet. This can result in returning an incorrect
value (null) or throwing ConflictingPersistentDataException from a get()
or put() on the region.
This change adds a check for persistent recovery to be completed
before creating the new bucket. If recovery isn't complete then the
operation on the region will fail with a PartitionOfflineException.
Queries on a region while persistent recovery is in progress can also
result in incorrect results so a similar check is added to
DefaultQuery.checkQueryOnPR.
New DUnit tests added for gets, puts, and queries for cases where persistent
colocated child regions haven't been started and where the child region
has been started but persistent recovery is still in progress when
the region operation is attempted.
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/cache/query/Query.java e27687d
geode-core/src/main/java/org/apache/geode/cache/query/internal/DefaultQuery.java 58df390
geode-core/src/main/java/org/apache/geode/internal/cache/PRHARedundancyProvider.java cfedb67
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java baab79f
geode-core/src/main/java/org/apache/geode/internal/i18n/LocalizedStrings.java 8bfdd68
geode-core/src/test/java/org/apache/geode/cache/query/partitioned/PRBasicQueryDUnitTest.java 8ef907a
geode-core/src/test/java/org/apache/geode/cache/query/partitioned/PRQueryDUnitHelper.java cfb4190
geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/PersistentColocatedPartitionedRegionDUnitTest.java 0a25228
Diff: https://reviews.apache.org/r/52891/diff/
Testing
-------
precheckin
Thanks,
Ken Howe