Re: Review Request 52891: GEODE-538: Add check for persistent data recovery

2016-10-18 Thread Ken Howe 

---
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

2016-10-18 Thread anilkumar gingade 

---
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)


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

2016-10-18 Thread Ken Howe 


> 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
> > 
> >
> > 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

2016-10-17 Thread Ken Howe 

---
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

Re: Review Request 52891: GEODE-538: Add check for persistent data recovery

2016-10-17 Thread anilkumar gingade 

---
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)


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)


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
> 
>

Review Request 52891: GEODE-538: Add check for persistent data recovery

2016-10-14 Thread Ken Howe 

---
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