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.

Sign up for GitHub

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

Jump to bottom

Add evictions metrics support for RedissonCache #4739

Merged
merged 1 commit into from Dec 14, 2022
Merged

Add evictions metrics support for RedissonCache #4739

merged 1 commit into from Dec 14, 2022

Conversation

nicdard
Copy link
Contributor

@nicdard nicdard commented Dec 13, 2022 •
edited

Currently, RedissonCache (aka the implementation org.springframework.cache.Cache) supports some metrics (hits, misses, puts) but evictions are not taken into account.

@nicdard nicdard force-pushed the feat/spring-cache-evictions-metric branch from ff7798d to e1dcd7f Compare December 13, 2022 13:36
@nicdard nicdard marked this pull request as ready for review December 13, 2022 13:37
mrniko
mrniko requested changes Dec 13, 2022
View reviewed changes
redisson/src/main/java/org/redisson/spring/cache/RedissonCache.java Outdated
@@ -46,11 +46,14 @@ public class RedissonCache implements Cache {
private final AtomicLong puts = new AtomicLong();

private final AtomicLong misses = new AtomicLong();

private AtomicLong evictions;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make it final and init in field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was initialising it in one of the constructor and not in the other one as I wanted to signal that in one case we can count evictions but not in the other one. Does it makes sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, which case? I think we need to calculate it the same way we do for other metrics

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes I see, I was wrong with my assumption, sorry, I'll fix this

redisson/src/main/java/org/redisson/spring/cache/RedissonCache.java Outdated

public RedissonCache(RMapCache<Object, Object> mapCache, CacheConfig config, boolean allowNullValues) {
this(mapCache, allowNullValues);
this.mapCache = mapCache;
this.config = config;
this.evictions = new AtomicLong();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be initialized in the field

redisson/src/main/java/org/redisson/spring/cache/RedissonCache.java Outdated
@@ -238,6 +242,12 @@ long getCacheMisses(){
long getCachePuts() {
return puts.get();
}

Long getCacheEvictions() {
return evictions != null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

evictions can't be null

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is null, when this cache is backed by RMap (created with: public RedissonCache(RMap<Object, Object> map, boolean allowNullValues) constructor)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is why I asked to init it in the field

redisson/src/main/java/org/redisson/spring/cache/RedissonCache.java Outdated
@@ -251,4 +261,9 @@ private void addCacheMiss(){
misses.incrementAndGet();
}

private void addCacheEvictions(long delta) {
if (this.evictions != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

evictions can't be null

@nicdard
Copy link
Contributor Author

nicdard commented Dec 13, 2022

@mrniko thanks a lot for the review! I added some consideration on why I am using a nullable evictions counter

However, I can add a boolean to signal whether the cache supports evictions, and in RedissonCacheMetrics I can then return null if the cache is not supporting eviction operations. This way I can initialise the atomic counter in the field. What do you think?

@mrniko
Copy link
Member

mrniko commented Dec 13, 2022 •
edited

@nicdard

Thanks for the contribution! But evict() method is a part of Cache interface this is why I think we need to collect evictions in any case. Please correct the implementation

@mrniko mrniko added this to the 3.18.2 milestone Dec 13, 2022
@mrniko mrniko added the bug label Dec 13, 2022
@nicdard nicdard force-pushed the feat/spring-cache-evictions-metric branch from e1dcd7f to 67407f0 Compare December 13, 2022 14:22
@nicdard
Copy link
Contributor Author

nicdard commented Dec 13, 2022

@nicdard

Thanks for the contribution! But evict() method is a part of Cache interface this is why I think we need to collect evictions in any case. Please correct the implementation

You are right, thanks! I amended my commit :)

@nicdard nicdard changed the title Add evictions metrics support for RedissonCache backed by RMapCache Add evictions metrics support for RedissonCache Dec 13, 2022
@nicdard
Copy link
Contributor Author

nicdard commented Dec 13, 2022

@mrniko As this is my first contribution, could I ask you how your release cycle works? What's the timeframe you usually allocate for a new release to come out?

@mrniko
Copy link
Member

mrniko commented Dec 13, 2022

That's depends on the importance of the changes made in release

@nicdard
Copy link
Contributor Author

nicdard commented Dec 13, 2022

That's depends on the importance of the changes made in release

Ok thanks!

feat: add evictions metrics support for RedissonCache
3b937e2 
Signed-off-by: Nicola Dardanis <ndardanis@adobe.com>
@nicdard nicdard force-pushed the feat/spring-cache-evictions-metric branch from 67407f0 to 3b937e2 Compare December 13, 2022 14:32
@mrniko mrniko merged commit f0b722c into redisson:master Dec 14, 2022
@nicdard nicdard deleted the feat/spring-cache-evictions-metric branch December 14, 2022 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrniko mrniko mrniko approved these changes

Assignees
No one assigned
Labels
bug
Milestone
3.19.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@nicdard @mrniko