You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Hitesh Khamesra <hk...@pivotal.io> on 2016/08/24 18:30:32 UTC
Review Request 51385: GEODE-900 test endup uding old cache
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51385/
-----------------------------------------------------------
Review request for geode, Bruce Schuchardt and Darrel Schneider.
Repository: geode
Description
-------
1. When cache is closed need to return null from "checkExistingCache"
2. Need to synchronize cache creation
Diffs
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java 76a7bad
Diff: https://reviews.apache.org/r/51385/diff/
Testing
-------
Thanks,
Hitesh Khamesra
Re: Review Request 51385: GEODE-900 test endup uding old cache
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51385/#review146696
-----------------------------------------------------------
Ship it!
Ship It!
- Udo Kohlmeyer
On Aug. 24, 2016, 6:30 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51385/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2016, 6:30 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Darrel Schneider.
>
>
> Repository: geode
>
>
> Description
> -------
>
> 1. When cache is closed need to return null from "checkExistingCache"
> 2. Need to synchronize cache creation
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java 76a7bad
>
> Diff: https://reviews.apache.org/r/51385/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 51385: GEODE-900 test endup uding old cache
Posted by Hitesh Khamesra <hk...@pivotal.io>.
> On Aug. 24, 2016, 8:27 p.m., Bruce Schuchardt wrote:
> > While it's okay to synchronize like that in checkExistingCache() I think it might be more appropriate to synchronize on CacheFactory.class in basicCreate() and fold the checkExistingClass() method into that method since that's the only place it's used. I'm concerned that the static "instance" variable is being set in GemFireCacheImpl.initialize() outside of synchronization. It used to be that there was only one path to basicCreate() and that method synchronized on CacheFactory.class, but now there appear to be other paths to it, like client-cache creation, that don't synchronize.
>>I'm concerned that the static "instance" variable is being set in GemFireCacheImpl.initialize() outside of synchronization.
This is now part of synchronization
synchronized (GemFireCacheImpl.class) {
GemFireCacheImpl instance = checkExistingCache(existingOk, cacheConfig);
if (instance == null) {
instance = new GemFireCacheImpl(isClient, pf, system, cacheConfig, asyncEventListeners, typeRegistry);
instance.initialize();
}
return instance;
}
- Hitesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51385/#review146705
-----------------------------------------------------------
On Aug. 24, 2016, 6:30 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51385/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2016, 6:30 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Darrel Schneider.
>
>
> Repository: geode
>
>
> Description
> -------
>
> 1. When cache is closed need to return null from "checkExistingCache"
> 2. Need to synchronize cache creation
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java 76a7bad
>
> Diff: https://reviews.apache.org/r/51385/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 51385: GEODE-900 test endup uding old cache
Posted by Bruce Schuchardt <bs...@pivotal.io>.
> On Aug. 24, 2016, 8:27 p.m., Bruce Schuchardt wrote:
> > While it's okay to synchronize like that in checkExistingCache() I think it might be more appropriate to synchronize on CacheFactory.class in basicCreate() and fold the checkExistingClass() method into that method since that's the only place it's used. I'm concerned that the static "instance" variable is being set in GemFireCacheImpl.initialize() outside of synchronization. It used to be that there was only one path to basicCreate() and that method synchronized on CacheFactory.class, but now there appear to be other paths to it, like client-cache creation, that don't synchronize.
>
> Hitesh Khamesra wrote:
> >>I'm concerned that the static "instance" variable is being set in GemFireCacheImpl.initialize() outside of synchronization.
> This is now part of synchronization
>
> synchronized (GemFireCacheImpl.class) {
> GemFireCacheImpl instance = checkExistingCache(existingOk, cacheConfig);
> if (instance == null) {
> instance = new GemFireCacheImpl(isClient, pf, system, cacheConfig, asyncEventListeners, typeRegistry);
> instance.initialize();
> }
> return instance;
> }
Yes, like that
- Bruce
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51385/#review146705
-----------------------------------------------------------
On Aug. 24, 2016, 6:30 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51385/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2016, 6:30 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Darrel Schneider.
>
>
> Repository: geode
>
>
> Description
> -------
>
> 1. When cache is closed need to return null from "checkExistingCache"
> 2. Need to synchronize cache creation
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java 76a7bad
>
> Diff: https://reviews.apache.org/r/51385/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 51385: GEODE-900 test endup uding old cache
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51385/#review146705
-----------------------------------------------------------
While it's okay to synchronize like that in checkExistingCache() I think it might be more appropriate to synchronize on CacheFactory.class in basicCreate() and fold the checkExistingClass() method into that method since that's the only place it's used. I'm concerned that the static "instance" variable is being set in GemFireCacheImpl.initialize() outside of synchronization. It used to be that there was only one path to basicCreate() and that method synchronized on CacheFactory.class, but now there appear to be other paths to it, like client-cache creation, that don't synchronize.
- Bruce Schuchardt
On Aug. 24, 2016, 6:30 p.m., Hitesh Khamesra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51385/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2016, 6:30 p.m.)
>
>
> Review request for geode, Bruce Schuchardt and Darrel Schneider.
>
>
> Repository: geode
>
>
> Description
> -------
>
> 1. When cache is closed need to return null from "checkExistingCache"
> 2. Need to synchronize cache creation
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java 76a7bad
>
> Diff: https://reviews.apache.org/r/51385/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Hitesh Khamesra
>
>
Re: Review Request 51385: GEODE-900 test endup using old cache
Posted by Hitesh Khamesra <hk...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51385/
-----------------------------------------------------------
(Updated Aug. 24, 2016, 10:01 p.m.)
Review request for geode, Bruce Schuchardt and Darrel Schneider.
Changes
-------
typo in summary
Summary (updated)
-----------------
GEODE-900 test endup using old cache
Repository: geode
Description
-------
1. When cache is closed need to return null from "checkExistingCache"
2. Need to synchronize cache creation
Diffs
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java 76a7bad
Diff: https://reviews.apache.org/r/51385/diff/
Testing
-------
Thanks,
Hitesh Khamesra