Re: Review Request 53442: GEODE-1740: Correct potential region inconsistencies with concurrent clear and transaction commit

2016-11-07 Thread Darrel Schneider 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53442/#review155228
---




geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java 
(line 3695)


I see no reason for the !r.hasServerProxy check



geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java 
(line 3704)


I think lockForTXCacheModification is now dead code


- Darrel Schneider


On Nov. 3, 2016, 4:08 p.m., Scott Jewell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53442/
> ---
> 
> (Updated Nov. 3, 2016, 4:08 p.m.)
> 
> 
> Review request for geode, Darrel Schneider and Eric Shu.
> 
> 
> Bugs: GEODE-1740
> https://issues.apache.org/jira/browse/GEODE-1740
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-1740: Correct potential region inconsistencies with concurrent clear 
> and transaction commit
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
>  de05b0d3991254834da94ed97ada9c9247aa69ab 
>   geode-core/src/main/java/org/apache/geode/internal/cache/RegionMap.java 
> ee8a84e57b5b72cc801cd2474a51dfd0bd3083f3 
>   geode-core/src/main/java/org/apache/geode/internal/cache/TXState.java 
> 99a3b83d931dccd0faec482e42cda7ded18a13e7 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/ClearTXLockingDUnitTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53442/diff/
> 
> 
> Testing
> ---
> 
> New unit test ClearTXLockingDUnitTest
> 
> precheckin
> 
> 
> Thanks,
> 
> Scott Jewell
> 
>

Re: Review Request 53442: GEODE-1740: Correct potential region inconsistencies with concurrent clear and transaction commit

2016-11-07 Thread anilkumar gingade 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53442/#review155212
---




geode-core/src/main/java/org/apache/geode/internal/cache/TXState.java (line 514)


What if one of the region is destroyed while iterating over the regions? 
Can we still operate on version vector (in lockRegionForAtomicTX())...Do we 
need to have some error handlling code?


- anilkumar gingade


On Nov. 3, 2016, 11:08 p.m., Scott Jewell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53442/
> ---
> 
> (Updated Nov. 3, 2016, 11:08 p.m.)
> 
> 
> Review request for geode, Darrel Schneider and Eric Shu.
> 
> 
> Bugs: GEODE-1740
> https://issues.apache.org/jira/browse/GEODE-1740
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-1740: Correct potential region inconsistencies with concurrent clear 
> and transaction commit
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
>  de05b0d3991254834da94ed97ada9c9247aa69ab 
>   geode-core/src/main/java/org/apache/geode/internal/cache/RegionMap.java 
> ee8a84e57b5b72cc801cd2474a51dfd0bd3083f3 
>   geode-core/src/main/java/org/apache/geode/internal/cache/TXState.java 
> 99a3b83d931dccd0faec482e42cda7ded18a13e7 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/ClearTXLockingDUnitTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53442/diff/
> 
> 
> Testing
> ---
> 
> New unit test ClearTXLockingDUnitTest
> 
> precheckin
> 
> 
> Thanks,
> 
> Scott Jewell
> 
>

Re: Review Request 53442: GEODE-1740: Correct potential region inconsistencies with concurrent clear and transaction commit

2016-11-07 Thread Darrel Schneider 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53442/#review155204
---


Ship it!




Ship It!

- Darrel Schneider


On Nov. 3, 2016, 4:08 p.m., Scott Jewell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53442/
> ---
> 
> (Updated Nov. 3, 2016, 4:08 p.m.)
> 
> 
> Review request for geode, Darrel Schneider and Eric Shu.
> 
> 
> Bugs: GEODE-1740
> https://issues.apache.org/jira/browse/GEODE-1740
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-1740: Correct potential region inconsistencies with concurrent clear 
> and transaction commit
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
>  de05b0d3991254834da94ed97ada9c9247aa69ab 
>   geode-core/src/main/java/org/apache/geode/internal/cache/RegionMap.java 
> ee8a84e57b5b72cc801cd2474a51dfd0bd3083f3 
>   geode-core/src/main/java/org/apache/geode/internal/cache/TXState.java 
> 99a3b83d931dccd0faec482e42cda7ded18a13e7 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/ClearTXLockingDUnitTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53442/diff/
> 
> 
> Testing
> ---
> 
> New unit test ClearTXLockingDUnitTest
> 
> precheckin
> 
> 
> Thanks,
> 
> Scott Jewell
> 
>

Re: Review Request 53442: GEODE-1740: Correct potential region inconsistencies with concurrent clear and transaction commit

2016-11-03 Thread Scott Jewell 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53442/
---

(Updated Nov. 3, 2016, 11:08 p.m.)


Review request for geode, Darrel Schneider and Eric Shu.


Changes
---

Added try/finally clause for lock/unlock of regions


Bugs: GEODE-1740
https://issues.apache.org/jira/browse/GEODE-1740


Repository: geode


Description
---

GEODE-1740: Correct potential region inconsistencies with concurrent clear and 
transaction commit


Diffs (updated)
-

  
geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java 
de05b0d3991254834da94ed97ada9c9247aa69ab 
  geode-core/src/main/java/org/apache/geode/internal/cache/RegionMap.java 
ee8a84e57b5b72cc801cd2474a51dfd0bd3083f3 
  geode-core/src/main/java/org/apache/geode/internal/cache/TXState.java 
99a3b83d931dccd0faec482e42cda7ded18a13e7 
  
geode-core/src/test/java/org/apache/geode/internal/cache/ClearTXLockingDUnitTest.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/53442/diff/


Testing
---

New unit test ClearTXLockingDUnitTest

precheckin


Thanks,

Scott Jewell

Re: Review Request 53442: GEODE-1740: Correct potential region inconsistencies with concurrent clear and transaction commit

2016-11-03 Thread Darrel Schneider 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53442/#review154828
---




geode-core/src/main/java/org/apache/geode/internal/cache/TXState.java (line 494)


Instead of using an existing finally block, I think you should add your own 
new one.
You should add a "try" right after you call lockTXRegions and then a 
finally that matches it that only calls unlockTXRegions. That way you do not 
end up calling unlockTXRegions if exceptions cause you to never calls 
lockTXRegions.


- Darrel Schneider


On Nov. 3, 2016, 3:01 p.m., Scott Jewell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53442/
> ---
> 
> (Updated Nov. 3, 2016, 3:01 p.m.)
> 
> 
> Review request for geode, Darrel Schneider and Eric Shu.
> 
> 
> Bugs: GEODE-1740
> https://issues.apache.org/jira/browse/GEODE-1740
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-1740: Correct potential region inconsistencies with concurrent clear 
> and transaction commit
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
>  de05b0d3991254834da94ed97ada9c9247aa69ab 
>   geode-core/src/main/java/org/apache/geode/internal/cache/RegionMap.java 
> ee8a84e57b5b72cc801cd2474a51dfd0bd3083f3 
>   geode-core/src/main/java/org/apache/geode/internal/cache/TXState.java 
> 99a3b83d931dccd0faec482e42cda7ded18a13e7 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/ClearTXLockingDUnitTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53442/diff/
> 
> 
> Testing
> ---
> 
> New unit test ClearTXLockingDUnitTest
> 
> precheckin
> 
> 
> Thanks,
> 
> Scott Jewell
> 
>

Review Request 53442: GEODE-1740: Correct potential region inconsistencies with concurrent clear and transaction commit

2016-11-03 Thread Scott Jewell 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53442/
---

Review request for geode, Darrel Schneider and Eric Shu.


Bugs: GEODE-1740
https://issues.apache.org/jira/browse/GEODE-1740


Repository: geode


Description
---

GEODE-1740: Correct potential region inconsistencies with concurrent clear and 
transaction commit


Diffs
-

  
geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java 
de05b0d3991254834da94ed97ada9c9247aa69ab 
  geode-core/src/main/java/org/apache/geode/internal/cache/RegionMap.java 
ee8a84e57b5b72cc801cd2474a51dfd0bd3083f3 
  geode-core/src/main/java/org/apache/geode/internal/cache/TXState.java 
99a3b83d931dccd0faec482e42cda7ded18a13e7 
  
geode-core/src/test/java/org/apache/geode/internal/cache/ClearTXLockingDUnitTest.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/53442/diff/


Testing
---

New unit test ClearTXLockingDUnitTest

precheckin


Thanks,

Scott Jewell