Re: Review Request 36594: Adding try-with-resources for Cache

2015-07-24 Thread Kirk Lund 

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

Ship it!


Ship It!

- Kirk Lund


On July 23, 2015, 6:19 a.m., William Markito wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36594/
> ---
> 
> (Updated July 23, 2015, 6:19 a.m.)
> 
> 
> Review request for geode, Darrel Schneider and Kirk Lund.
> 
> 
> Bugs: GEODE-144
> https://issues.apache.org/jira/browse/GEODE-144
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Adding try-with-resources for Cache
> 
> 
> Diffs
> -
> 
>   gemfire-core/src/main/java/com/gemstone/gemfire/cache/RegionService.java 
> 9ea65c0 
>   
> gemfire-core/src/test/java/com/gemstone/gemfire/cache/RegionFactoryJUnitTest.java
>  151e9c8 
> 
> Diff: https://reviews.apache.org/r/36594/diff/
> 
> 
> Testing
> ---
> 
> Added JUnit test
> Ran test suite/build.
> 
> 
> Thanks,
> 
> William Markito
> 
>

Re: Review Request 36594: Adding try-with-resources for Cache

2015-07-23 Thread Kirk Lund 

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

Ship it!


Ship It!

- Kirk Lund


On July 23, 2015, 6:19 a.m., William Markito wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36594/
> ---
> 
> (Updated July 23, 2015, 6:19 a.m.)
> 
> 
> Review request for geode, Darrel Schneider and Kirk Lund.
> 
> 
> Bugs: GEODE-144
> https://issues.apache.org/jira/browse/GEODE-144
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Adding try-with-resources for Cache
> 
> 
> Diffs
> -
> 
>   gemfire-core/src/main/java/com/gemstone/gemfire/cache/RegionService.java 
> 9ea65c0 
>   
> gemfire-core/src/test/java/com/gemstone/gemfire/cache/RegionFactoryJUnitTest.java
>  151e9c8 
> 
> Diff: https://reviews.apache.org/r/36594/diff/
> 
> 
> Testing
> ---
> 
> Added JUnit test
> Ran test suite/build.
> 
> 
> Thanks,
> 
> William Markito
> 
>

Re: Review Request 36594: Adding try-with-resources for Cache

2015-07-23 Thread Darrel Schneider 

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

Ship it!


Ship It!

- Darrel Schneider


On July 22, 2015, 11:19 p.m., William Markito wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36594/
> ---
> 
> (Updated July 22, 2015, 11:19 p.m.)
> 
> 
> Review request for geode, Darrel Schneider and Kirk Lund.
> 
> 
> Bugs: GEODE-144
> https://issues.apache.org/jira/browse/GEODE-144
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Adding try-with-resources for Cache
> 
> 
> Diffs
> -
> 
>   gemfire-core/src/main/java/com/gemstone/gemfire/cache/RegionService.java 
> 9ea65c0 
>   
> gemfire-core/src/test/java/com/gemstone/gemfire/cache/RegionFactoryJUnitTest.java
>  151e9c8 
> 
> Diff: https://reviews.apache.org/r/36594/diff/
> 
> 
> Testing
> ---
> 
> Added JUnit test
> Ran test suite/build.
> 
> 
> Thanks,
> 
> William Markito
> 
>

Re: Review Request 36594: Adding try-with-resources for Cache

2015-07-22 Thread William Markito 

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

(Updated July 23, 2015, 6:19 a.m.)


Review request for geode, Darrel Schneider and Kirk Lund.


Changes
---

Updated tests


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


Repository: geode


Description
---

Adding try-with-resources for Cache


Diffs (updated)
-

  gemfire-core/src/main/java/com/gemstone/gemfire/cache/RegionService.java 
9ea65c0 
  
gemfire-core/src/test/java/com/gemstone/gemfire/cache/RegionFactoryJUnitTest.java
 151e9c8 

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


Testing
---

Added JUnit test
Ran test suite/build.


Thanks,

William Markito

Re: Review Request 36594: Adding try-with-resources for Cache

2015-07-22 Thread William Markito 


> On July 20, 2015, 5:25 p.m., Darrel Schneider wrote:
> > gemfire-core/src/test/java/com/gemstone/gemfire/cache/RegionFactoryJUnitTest.java,
> >  line 1113
> > 
> >
> > I'm not sure this unit test needs to create a region and then call 
> > assertBasicRegionFunctionality.
> > Why don't you just make sure that the cache is closed after the try 
> > block?
> > 
> > Inside the try block you could assert that !cache.isClosed().
> > Outside the try block you could assert that cache.isClosed.
> > You just need add a "Cache c1;" outside the try block and then "c1 = 
> > cache;" inside the try block.
> > The after the try block you can assert that c1.isClosed().
> > I'd get rid of all the RegionFactory code get rid of this test throwing 
> > CacheClosedException

Thanks Darrel! I was following some of the patterns at testAfterCacheClosed, 
testAfterConnect, testRegionFactoryAndCacheClose.. But I more than agree that 
it's not necessary. Fixed.


- William


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


On July 18, 2015, 5:18 a.m., William Markito wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36594/
> ---
> 
> (Updated July 18, 2015, 5:18 a.m.)
> 
> 
> Review request for geode, Darrel Schneider and Kirk Lund.
> 
> 
> Bugs: GEODE-144
> https://issues.apache.org/jira/browse/GEODE-144
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Adding try-with-resources for Cache
> 
> 
> Diffs
> -
> 
>   gemfire-core/src/main/java/com/gemstone/gemfire/cache/RegionService.java 
> 9ea65c0 
>   
> gemfire-core/src/test/java/com/gemstone/gemfire/cache/RegionFactoryJUnitTest.java
>  151e9c8 
> 
> Diff: https://reviews.apache.org/r/36594/diff/
> 
> 
> Testing
> ---
> 
> Added JUnit test
> Ran test suite/build.
> 
> 
> Thanks,
> 
> William Markito
> 
>

Re: Review Request 36594: Adding try-with-resources for Cache

2015-07-20 Thread Darrel Schneider 

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



gemfire-core/src/test/java/com/gemstone/gemfire/cache/RegionFactoryJUnitTest.java
 (line 1113)


I'm not sure this unit test needs to create a region and then call 
assertBasicRegionFunctionality.
Why don't you just make sure that the cache is closed after the try block?

Inside the try block you could assert that !cache.isClosed().
Outside the try block you could assert that cache.isClosed.
You just need add a "Cache c1;" outside the try block and then "c1 = 
cache;" inside the try block.
The after the try block you can assert that c1.isClosed().
I'd get rid of all the RegionFactory code get rid of this test throwing 
CacheClosedException


- Darrel Schneider


On July 17, 2015, 10:18 p.m., William Markito wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36594/
> ---
> 
> (Updated July 17, 2015, 10:18 p.m.)
> 
> 
> Review request for geode, Darrel Schneider and Kirk Lund.
> 
> 
> Bugs: GEODE-144
> https://issues.apache.org/jira/browse/GEODE-144
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> Adding try-with-resources for Cache
> 
> 
> Diffs
> -
> 
>   gemfire-core/src/main/java/com/gemstone/gemfire/cache/RegionService.java 
> 9ea65c0 
>   
> gemfire-core/src/test/java/com/gemstone/gemfire/cache/RegionFactoryJUnitTest.java
>  151e9c8 
> 
> Diff: https://reviews.apache.org/r/36594/diff/
> 
> 
> Testing
> ---
> 
> Added JUnit test
> Ran test suite/build.
> 
> 
> Thanks,
> 
> William Markito
> 
>

Review Request 36594: Adding try-with-resources for Cache

2015-07-17 Thread William Markito 

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

Review request for geode, Darrel Schneider and Kirk Lund.


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


Repository: geode


Description
---

Adding try-with-resources for Cache


Diffs
-

  gemfire-core/src/main/java/com/gemstone/gemfire/cache/RegionService.java 
9ea65c0 
  
gemfire-core/src/test/java/com/gemstone/gemfire/cache/RegionFactoryJUnitTest.java
 151e9c8 

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


Testing
---

Added JUnit test
Ran test suite/build.


Thanks,

William Markito