Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes handling of cache statistics #15937

Merged
merged 2 commits into from Nov 6, 2019

Conversation

@vbekiaris
Copy link
Contributor

vbekiaris commented Nov 5, 2019

Previously used Config.findCacheConfig can only lookup
cache configs added statically or dynamically via Config.addCacheConfig
but misses configuration of dynamically created caches via
CacheManager.createCache, so it is wrong to use it to filter stats.
Instead, we retrieve cache configs from CacheService.getCacheConfigs
which includes configuration of all caches known at runtime.

Fixes missing cache statistics from MC

@vbekiaris vbekiaris added this to the 4.0 milestone Nov 5, 2019
@vbekiaris vbekiaris requested review from emre-aydin and blazember Nov 5, 2019
@vbekiaris vbekiaris self-assigned this Nov 5, 2019
Previously used Config.findCacheConfig can only lookup
cache configs added statically or dynamically via Config.addCacheConfig
but misses configuration of dynamically created caches via
CacheManager.createCache, so it is wrong to use it to filter stats.
Instead, we retrieve cache configs from CacheService.getCacheConfigs
which includes configuration of all known caches at runtime.
@vbekiaris vbekiaris force-pushed the vbekiaris:fixes/4.0/cache-stats-mc branch from e2719b9 to 2724b90 Nov 5, 2019
Map<String, LocalCacheStats> stats = cacheService.getStats();
for (String cacheNameWithPrefix : stats.keySet()) {
assertContains(cacheNameWithPrefix, CACHE_WITH_STATS_PREFIX);
}

This comment has been minimized.

Copy link
@blazember

blazember Nov 5, 2019

Contributor

Don't we need an assertEquals(100,stats.size()) too? It's verified by the next case though that the stat disabled caches actually don't have stats. Do we need then the 50 stats disabled caches in this test?

This comment has been minimized.

Copy link
@vbekiaris

vbekiaris Nov 6, 2019

Author Contributor

I pushed a commit removing the redundant test.

@emre-aydin emre-aydin merged commit 7dfc524 into hazelcast:master Nov 6, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@vbekiaris vbekiaris deleted the vbekiaris:fixes/4.0/cache-stats-mc branch Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.