-
Notifications
You must be signed in to change notification settings - Fork 37
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
Redis cache connection pool #273
Conversation
… types for variables
Looking into the sonar issues |
The duplication smell is going to be a significant change afaics 🤔 Not sure how far we go with this to appease Sonar... I'll try and get the coverage up a bit |
There may be a bug in |
SonarCloud Quality Gate failed. |
@graemerocher I am struggling to find non-contrived solutions for the Sonar Gate. I have added more sensible coverage and found (I believe) a bug in the async invalidateAll Removing the duplication would (imo) complicate the implementation Unless you can see a route to deduplication, or any coverage we need to have? |
Ok we can probably just merge as is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timyates can you address these comments in a separate PR?
...c/main/java/io/micronaut/configuration/lettuce/AbstractRedisConnectionPoolConfiguration.java
Show resolved
Hide resolved
...c/main/java/io/micronaut/configuration/lettuce/AbstractRedisConnectionPoolConfiguration.java
Show resolved
Hide resolved
...c/main/java/io/micronaut/configuration/lettuce/AbstractRedisConnectionPoolConfiguration.java
Show resolved
Hide resolved
...c/main/java/io/micronaut/configuration/lettuce/AbstractRedisConnectionPoolConfiguration.java
Show resolved
Hide resolved
...c/main/java/io/micronaut/configuration/lettuce/AbstractRedisConnectionPoolConfiguration.java
Show resolved
Hide resolved
.../src/main/java/io/micronaut/configuration/lettuce/cache/RedisAsyncConnectionPoolFactory.java
Show resolved
Hide resolved
.../src/main/java/io/micronaut/configuration/lettuce/cache/RedisAsyncConnectionPoolFactory.java
Show resolved
Hide resolved
.../src/main/java/io/micronaut/configuration/lettuce/cache/RedisAsyncConnectionPoolFactory.java
Show resolved
Hide resolved
* @param key The key | ||
* @return bytes of the object | ||
*/ | ||
protected byte[] serializeKey(Object key) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deleting this method is a breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the japicmp task had it down as a "source compatible" warning (not an error)
I added them back in anyway, and found the Sonar incantation to stop it complaining
...lettuce/src/main/java/io/micronaut/configuration/lettuce/cache/RedisConnectionPoolCache.java
Show resolved
Hide resolved
This addresses the feedback from @sdelamo in #273 Two items remain: 1. [don't use Optional as method parameter](https://github.com/micronaut-projects/micronaut-redis/pull/273\#discussion_r826692810) 2. [deleting this method is a breaking change](https://github.com/micronaut-projects/micronaut-redis/pull/273\#discussion_r826696245)
* Address feedback for connection pool This addresses the feedback from @sdelamo in #273 Two items remain: 1. [don't use Optional as method parameter](https://github.com/micronaut-projects/micronaut-redis/pull/273\#discussion_r826692810) 2. [deleting this method is a breaking change](https://github.com/micronaut-projects/micronaut-redis/pull/273\#discussion_r826696245) * Binary compatability in RedisCache
Update of #217 merging in the current master
Also removed unused imports from the test spec
@graemerocher as far as I can see, your review has been addressed. And all tests pass.
Is this good to go?
@sdelamo I've assigned this to the next minor release, but now I'm thinking it should be a major for the changes to RedisCache.java