Re: Review Request 53071: GEODE-2021: Get on non colocated keys in a transaction does not throw TransactionDataNotColocatedException

2016-10-21 Thread Darrel Schneider 

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




geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java
 (line 428)


change "isCust1Local ? false : true" to "!isCust1Local"



geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java
 (line 432)


change "isCust1Local ? true : false" to "isCust1Local"


- Darrel Schneider


On Oct. 21, 2016, 10:08 a.m., Eric Shu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53071/
> ---
> 
> (Updated Oct. 21, 2016, 10:08 a.m.)
> 
> 
> Review request for geode, Darrel Schneider and Swapnil Bawaskar.
> 
> 
> Bugs: GEODE-2021
> https://issues.apache.org/jira/browse/GEODE-2021
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Throw TransactionDataNotColocatedException when get locally failed with 
> BucketNotFoundException
> Added a dunit test which passes with the fix and failed without the fix.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
>  baab79f 
>   geode-core/src/test/java/org/apache/geode/disttx/PRDistTXDUnitTest.java 
> f36085b 
>   
> geode-core/src/test/java/org/apache/geode/disttx/PRDistTXWithVersionsDUnitTest.java
>  268c2ed 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRColocationDUnitTest.java
>  1b8d2d1 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java
>  516c240 
> 
> Diff: https://reviews.apache.org/r/53071/diff/
> 
> 
> Testing
> ---
> 
> precheckin.
> 
> 
> Thanks,
> 
> Eric Shu
> 
>

Re: Review Request 53071: GEODE-2021: Get on non colocated keys in a transaction does not throw TransactionDataNotColocatedException

2016-10-21 Thread anilkumar gingade 

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


Ship it!




Ship It!

- anilkumar gingade


On Oct. 21, 2016, 5:08 p.m., Eric Shu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53071/
> ---
> 
> (Updated Oct. 21, 2016, 5:08 p.m.)
> 
> 
> Review request for geode, Darrel Schneider and Swapnil Bawaskar.
> 
> 
> Bugs: GEODE-2021
> https://issues.apache.org/jira/browse/GEODE-2021
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Throw TransactionDataNotColocatedException when get locally failed with 
> BucketNotFoundException
> Added a dunit test which passes with the fix and failed without the fix.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
>  baab79f 
>   geode-core/src/test/java/org/apache/geode/disttx/PRDistTXDUnitTest.java 
> f36085b 
>   
> geode-core/src/test/java/org/apache/geode/disttx/PRDistTXWithVersionsDUnitTest.java
>  268c2ed 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRColocationDUnitTest.java
>  1b8d2d1 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java
>  516c240 
> 
> Diff: https://reviews.apache.org/r/53071/diff/
> 
> 
> Testing
> ---
> 
> precheckin.
> 
> 
> Thanks,
> 
> Eric Shu
> 
>

Re: Review Request 53071: GEODE-2021: Get on non colocated keys in a transaction does not throw TransactionDataNotColocatedException

2016-10-21 Thread Eric Shu 

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

(Updated Oct. 21, 2016, 5:08 p.m.)


Review request for geode, Darrel Schneider and Swapnil Bawaskar.


Changes
---

Refactoring test based on review comments.


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


Repository: geode


Description
---

Throw TransactionDataNotColocatedException when get locally failed with 
BucketNotFoundException
Added a dunit test which passes with the fix and failed without the fix.


Diffs (updated)
-

  
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java 
baab79f 
  geode-core/src/test/java/org/apache/geode/disttx/PRDistTXDUnitTest.java 
f36085b 
  
geode-core/src/test/java/org/apache/geode/disttx/PRDistTXWithVersionsDUnitTest.java
 268c2ed 
  
geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRColocationDUnitTest.java
 1b8d2d1 
  
geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java
 516c240 

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


Testing
---

precheckin.


Thanks,

Eric Shu

Review Request 53071: GEODE-2021: Get on non colocated keys in a transaction does not throw TransactionDataNotColocatedException

2016-10-20 Thread Eric Shu 

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

Review request for geode, Darrel Schneider and Swapnil Bawaskar.


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


Repository: geode


Description
---

Throw TransactionDataNotColocatedException when get locally failed with 
BucketNotFoundException
Added a dunit test which passes with the fix and failed without the fix.


Diffs
-

  
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java 
baab79f 
  
geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRColocationDUnitTest.java
 1b8d2d1 
  
geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java
 516c240 

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


Testing
---

precheckin.


Thanks,

Eric Shu

Re: Review Request 53071: GEODE-2021: Get on non colocated keys in a transaction does not throw TransactionDataNotColocatedException

2016-10-20 Thread Darrel Schneider 

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




geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java
 (line 451)


How about moving the rollback call to the "getTx" method?
mgr.begin();
boolean doRollback = true;
try {
 pr.get(cust1):
 pr.get(cust2);
 doRollback = false;
} finally {
  if (doRollback) {
mgr.rollback();
  } else {
mgr.commit();
  }
}



geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java
 (line 456)


how about changing this method to just be "getTx" and pass in a boolean 
that says "doCust1First".
Then do this:
CustId first = doCust1First ? cust1 : cust2;
CustId second = !doCust1First ? cust1 : cust2;

mgr.begin();
pr.get(first);
pr.get(second);
mgr commit();


- Darrel Schneider


On Oct. 20, 2016, 4:29 p.m., Eric Shu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53071/
> ---
> 
> (Updated Oct. 20, 2016, 4:29 p.m.)
> 
> 
> Review request for geode, Darrel Schneider and Swapnil Bawaskar.
> 
> 
> Bugs: GEODE-2021
> https://issues.apache.org/jira/browse/GEODE-2021
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Throw TransactionDataNotColocatedException when get locally failed with 
> BucketNotFoundException
> Added a dunit test which passes with the fix and failed without the fix.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
>  baab79f 
>   geode-core/src/test/java/org/apache/geode/disttx/PRDistTXDUnitTest.java 
> f36085b 
>   
> geode-core/src/test/java/org/apache/geode/disttx/PRDistTXWithVersionsDUnitTest.java
>  268c2ed 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRColocationDUnitTest.java
>  1b8d2d1 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java
>  516c240 
> 
> Diff: https://reviews.apache.org/r/53071/diff/
> 
> 
> Testing
> ---
> 
> precheckin.
> 
> 
> Thanks,
> 
> Eric Shu
> 
>

Re: Review Request 53071: GEODE-2021: Get on non colocated keys in a transaction does not throw TransactionDataNotColocatedException

2016-10-20 Thread Eric Shu 

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

(Updated Oct. 20, 2016, 11:29 p.m.)


Review request for geode, Darrel Schneider and Swapnil Bawaskar.


Changes
---

Using new Assertions to avoid try catch blocks.
Disalbe the dunit test to be run in dist tx tests.


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


Repository: geode


Description
---

Throw TransactionDataNotColocatedException when get locally failed with 
BucketNotFoundException
Added a dunit test which passes with the fix and failed without the fix.


Diffs (updated)
-

  
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java 
baab79f 
  geode-core/src/test/java/org/apache/geode/disttx/PRDistTXDUnitTest.java 
f36085b 
  
geode-core/src/test/java/org/apache/geode/disttx/PRDistTXWithVersionsDUnitTest.java
 268c2ed 
  
geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRColocationDUnitTest.java
 1b8d2d1 
  
geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java
 516c240 

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


Testing
---

precheckin.


Thanks,

Eric Shu

Re: Review Request 53071: GEODE-2021: Get on non colocated keys in a transaction does not throw TransactionDataNotColocatedException

2016-10-20 Thread Darrel Schneider 

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




geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java
 (line 439)


I think this test would be a bit clearer if you get rid of the boolean 
gotExpectedTransactionDataNotColocatedException
and instead just add a fail("expected 
TransactionDataNotColocatedException"); before the catch.

I think an even better way exists of doing this that gets rid of the 
try/catch. One or more of the unit test frameworks has ExpectedException 
support but I don't remember exactly how it works. But that would be preferred 
if you can find an example of how to use it.


- Darrel Schneider


On Oct. 20, 2016, 10:39 a.m., Eric Shu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53071/
> ---
> 
> (Updated Oct. 20, 2016, 10:39 a.m.)
> 
> 
> Review request for geode, Darrel Schneider and Swapnil Bawaskar.
> 
> 
> Bugs: GEODE-2021
> https://issues.apache.org/jira/browse/GEODE-2021
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Throw TransactionDataNotColocatedException when get locally failed with 
> BucketNotFoundException
> Added a dunit test which passes with the fix and failed without the fix.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
>  baab79f 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRColocationDUnitTest.java
>  1b8d2d1 
>   
> geode-core/src/test/java/org/apache/geode/internal/cache/execute/PRTransactionDUnitTest.java
>  516c240 
> 
> Diff: https://reviews.apache.org/r/53071/diff/
> 
> 
> Testing
> ---
> 
> precheckin.
> 
> 
> Thanks,
> 
> Eric Shu
> 
>