You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@geode.apache.org by Darrel Schneider <ds...@pivotal.io> on 2016/05/25 21:42:23 UTC

Review Request 47855: fix race in GemFireCacheImplTest

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

Review request for geode, Ken Howe and Sai Boorlagadda.


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


Repository: geode


Description
-------

fix race in GemFireCacheImplTest

The race was caused by the GemFireCacheImpl constructor creating a real TypeRegistry.
The TypeRegistry create a region which scheduled an async create region event with the
event pool. So when the test set "initialCount" this create region event might not have
completed so the initialCount would be zero. The test then schedule MAX_THREADS tasks
and waits until that many complete. But the create region event could also complete causing
getCompletedTaskCount() to keep return a value 1 more than the test expected.

The test now mocks the TypeRegistry so the only events scheduled with the pool come from
the unit test.


Diffs
-----

  geode-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java 3ff162e692c5e9e14e1d6e2dd16fa8d5e3e02bef 
  geode-core/src/test/java/com/gemstone/gemfire/internal/cache/GemFireCacheImplTest.java 71b7fc6aecf77244a226b4853a1f5992b4de35af 

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


Testing
-------

precheckin


Thanks,

Darrel Schneider

Re: Review Request 47855: fix race in GemFireCacheImplTest

Posted by Ken Howe <kh...@pivotal.io>.

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


Ship it!




Discussed an alternate implementation for adding TypeRegistry for unit testing:
Refactor, leaving the original method signatures for basicCreate(), createWithAsyncEventListeners(), and GemFireCachecImpl constructor, and add new methods with the new TypeRegistry parameter. Advantage would be leaving existing calls in product code as is, and use the TypeRegistry parameter (which is effectively a test hook) in the unit test.

That said, all of the changes in GemFireCachImpl are in private methods/contrustors, so the signature change is not exposed outside of the class. Code as submitted for review looks good to go.

- Ken Howe


On May 25, 2016, 9:42 p.m., Darrel Schneider wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47855/
> -----------------------------------------------------------
> 
> (Updated May 25, 2016, 9:42 p.m.)
> 
> 
> Review request for geode, Ken Howe and Sai Boorlagadda.
> 
> 
> Bugs: GEODE-1456
>     https://issues.apache.org/jira/browse/GEODE-1456
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> fix race in GemFireCacheImplTest
> 
> The race was caused by the GemFireCacheImpl constructor creating a real TypeRegistry.
> The TypeRegistry create a region which scheduled an async create region event with the
> event pool. So when the test set "initialCount" this create region event might not have
> completed so the initialCount would be zero. The test then schedule MAX_THREADS tasks
> and waits until that many complete. But the create region event could also complete causing
> getCompletedTaskCount() to keep return a value 1 more than the test expected.
> 
> The test now mocks the TypeRegistry so the only events scheduled with the pool come from
> the unit test.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java 3ff162e692c5e9e14e1d6e2dd16fa8d5e3e02bef 
>   geode-core/src/test/java/com/gemstone/gemfire/internal/cache/GemFireCacheImplTest.java 71b7fc6aecf77244a226b4853a1f5992b4de35af 
> 
> Diff: https://reviews.apache.org/r/47855/diff/
> 
> 
> Testing
> -------
> 
> precheckin
> 
> 
> Thanks,
> 
> Darrel Schneider
> 
>