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

Redis cache support #10057

Merged
merged 12 commits into from Sep 22, 2019
Merged

Conversation

Shaolans
Copy link
Member

@Shaolans Shaolans commented Jul 8, 2019

  • Please make sure the below checklist is followed for Pull Requests.

  • Travis tests are green

  • Tests are added where necessary

  • Documentation is added/updated where necessary

  • Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

This is my implementation of redis cache for the generator. Here you can check a example of an application generated with redis cache https://github.com/Shaolans/jhipster-app-redis-sample.
It uses this branch of Jhipster jhipster/jhipster#324.
Fix #9280

Copy link
Member

@hdurix hdurix left a comment

Choose a reason for hiding this comment

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

Great! I tried it and the cache is correctly used.

2 other things:

  • you could change the logger for redisson <logger name="org.redisson" level="WARN"/> as it's done for infinispan (a lot of logs are currently shown)
  • it would be nice to have the information in the readme that a redis instance has to be launched with Docker Compose in order to start the app

@hdurix
Copy link
Member

hdurix commented Jul 9, 2019

@Tcharl @jdubois @anthonyrichir @PierreBesson could you have a look at this PR?

@PierreBesson
Copy link
Contributor

There is something strange. It looks like the jhipsterProperties doesn't have the property: redis.enabled like what was done for memcached. I think we should stay consistent between the 2 implementations.

@Shaolans
Copy link
Member Author

Shaolans commented Jul 9, 2019

@PierreBesson We currently only supporting enabling/disabling cache on memcached not for others. Memcached is using Spring Cache and it exists NoOpCacheManager in order to disable the caching. However since this implementation is using JCache I am not sure if we have something similar.

@DanielFran
Copy link
Member

DanielFran commented Jul 9, 2019

Normally it is just necessary to add to app.yml:

spring:
  cache:
    type: none

See https://docs.spring.io/spring-boot/docs/current/reference/html/boot-features-caching.html#boot-features-caching-provider-none

@hdurix
Copy link
Member

hdurix commented Jul 9, 2019

So nothing to do in our hand? Except maybe to explain it in the README?

@Shaolans
Copy link
Member Author

Shaolans commented Jul 10, 2019

@DanielFran Nice thanks !

@hdurix I think so, here we just need to avoid to instantiate a RedisClient when the cache is none but if the cache is disabled, the user should also manually disable hibernate 2nd cache level if there is one since hibernate share the same CacheManager

@DanielFran
Copy link
Member

@Shaolans I do not believe you need to add your last commit since the cache will not be used if deactivated in configuration file.
I believe we just need to document it and make reference to spring documentation.

@Shaolans
Copy link
Member Author

Shaolans commented Jul 10, 2019

@DanielFran Even if we disable the cache, I think it will still instantiate a redis client and the HibernatePropertiesCustomizer bean will error since there is no cacheManager. Or is it up to the dev to do it ?

EDIT: I removed the commit.

Copy link

@JackyC415 JackyC415 left a comment

Choose a reason for hiding this comment

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

@Shaolans I'm just wondering how you were able to build the project when getRedis() is undefined? Thanks.

@Shaolans
Copy link
Member Author

@JackyC415 Hello you need to build the jhipster server library first by running ./mvnw clean install -Dgpg.skip=true -DskipTests on this branch jhipster/jhipster#324

@DanielFran DanielFran mentioned this pull request Jul 22, 2019
4 tasks
@pascalgrimaud
Copy link
Member

@Shaolans : just tested it and it works well. Good job ! But I have a comment:

  • it looks like I need to start a Redis server before lauching tests, otherwise, all tests will fail
  • is it the case ? Can't we use an embedded Redis server or maybe use testcontainers ?

@pascalgrimaud
Copy link
Member

Another comments too:

  • the docs at jhipster.tech need to be updated too
  • the option should work for JDL

@pascalgrimaud
Copy link
Member

If possible, once it's merged, if you can add a dedicated build at https://github.com/hipster-labs/jhipster-daily-builds it would be great !

@Shaolans
Copy link
Member Author

Shaolans commented Aug 3, 2019

it looks like I need to start a Redis server before lauching tests, otherwise, all tests will fail
is it the case ? Can't we use an embedded Redis server or maybe use testcontainers ?

You are totally right ! @pascalgrimaud According to you which one do you think we should use ?

the docs at jhipster.tech need to be updated too

I was planning on doing it, if the PR was good enough.

the option should work for JDL

Thanks for reminding me it, I totally forgot that case.

If possible, once it's merged, if you can add a dedicated build at https://github.com/hipster-labs/jhipster-daily-builds it would be great !

I was planning do it, once merge :) !

I will probably work on these points next week.

@pascalgrimaud
Copy link
Member

@Shaolans : Thanks. What I prefer in order:

  • I don't look how memcached works, it would be nice to be consistent
  • embedded server
  • disable cache for tests
  • testcontainer

@DanielFran
Copy link
Member

@jdubois Do you have time to review this PR since you were interested in?

@TonyLuo
Copy link
Contributor

TonyLuo commented Sep 10, 2019

any update on this? seems still not merged into master branch yet...

@pascalgrimaud
Copy link
Member

@TonyLuo : there is still an issue with integration tests, that's why it's not ready to be merged yet

@atomfrede
Copy link
Member

Good work @Shaolans really looking forward to have it merged!

@pascalgrimaud
Copy link
Member

Very good job @Shaolans !

@pascalgrimaud pascalgrimaud merged commit b4436aa into jhipster:master Sep 22, 2019
@Shaolans
Copy link
Member Author

@pascalgrimaud pascalgrimaud added this to the 6.4.0 milestone Oct 8, 2019
@pascalgrimaud pascalgrimaud mentioned this pull request Jan 16, 2020
1 task
@Shaolans Shaolans deleted the redis-cache-support branch April 10, 2020 08:57
@sinaghazi
Copy link

for some reason on version v6.10.4 I faced an issue with ExtendWith annotation not being imported in the AccountResourceIT class. My config is Okta and i used Redis for cache.

@pascalgrimaud
Copy link
Member

@sinaghazi : plz open a new ticket with all information so we can try to reproduce

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Redis support for Spring cache abstraction
9 participants