-
-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add caffeine cache implementation #117
Conversation
83253a1
to
254258b
Compare
@nshaw @joewhite101 |
The Quarkus properties have not been added for the moment. We could do it by 2 ways:
I have no idea, if we should provide default configuration or not. FYI in JHipster we have Java configuration that define default values provided by the JHipster framework: https://github.com/jhipster/generator-jhipster/blob/main/generators/server/templates/src/main/java/package/config/CacheConfiguration.java.ejs#L203 it's a bit different because this Java class configuration while we have properties configuration in Quarkus. /discuss |
@avdev4j , 1) I like the test refactoring, less code especially in this area is always good. 2) I'd favor defaulting to prod-ready settings which would include a default cache configuration. (I assume a cluster-aware implementation, e.g. redis, is also on the roadmap.) I don't think I know enough in this area to comment on which option is better but providing a default will help get a developer one step closer to a full application. |
This is my main issue right now. I think the best thing to do is to set caches with default values just like JHipster does.
We need to do this for:
I will do it tomorrow ;) |
Sounds like a good plan, thanks! |
Oh and yes, we need to provide a least a solution for distributed cache implementation. Unfortunately, it seems Quarkus does not provide an abstraction for this kind of scenario. The only solution seems to be using a client librairie. I have in mind redis, Infinispan and Hazelcast. I would like to do it for the next PR, It will be better to separate those from the current one. |
@joewhite101 probably has an opinion but we have Infinispan for Entando 6.2 and are adding redis for 6.3, so those would be my preferences, with Hazelcast in third place. Definitely agree, doing that in its own PR makes sense. This one already has a lot. |
I agree, according to main JHipster features I would say:
Redis seems to be quite used by cloud services too but I don't master this point so only suppositions. |
How JHipster deal with Spring Cache in Main generator:
It's easy to understand that both applicative and hibernate 2nd level cache share the same implementation, so if we use a distributed or centralized one we can't have issue with synchronization between different instances. Also, the code won't change because it will always use the Spring annotations, only the configuration part will be different according to the chosen solution. Unfortunately, this is not possible in Quarkus. We have Quarkus cache but it has only one impl with Caffeine. If we use that with Hibernate 2nd level cache we would not have issue because everything is local. For me we coud only do that :
How JHipster Quarkus will deal with Caching:On app generation
On entity generation e.g. fooFor this part nothing will be done with Caffeine, it's up to developer to do something close we have in User/UserService. Applicative cache is more related to business logic.
|
@nshaw @joewhite101 @danielpetisme I wrote this documentation to explain a bit more how this works, hope it will help (we could update the readme to add that). |
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.
LGTM, Awesome work
Regarding the caching doc, should it be done in the README.MD or in an external documentation |
092b46a
to
dbbbe34
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
#9