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

Provide a way to disable cleanup cron #1159

Closed
saiya opened this issue Aug 17, 2018 · 3 comments
Closed

Provide a way to disable cleanup cron #1159

saiya opened this issue Aug 17, 2018 · 3 comments
Assignees
Labels
status: duplicate A duplicate of another issue

Comments

@saiya
Copy link

saiya commented Aug 17, 2018

Hi, I want a way to disable cleanup cron in Spring Session Data Redis (as described in previous conversation: #1119 (comment) ).

Here is a issue that describes about the matter.

TL;DR

  • Sometime user want to disable cleanup cron of Spring Session Data Redis and Spring Session JDBC
  • There was a known workaround to set cleanup-cron=0 0 5 31 2 (February 31st, never comes), but it does not work with recent version of spring-context
  • It is difficult to override RedisHttpSessionConfiguration and JdbcHttpSessionConfiguration to prevent cleanup cron.
  • What I want is a way to disable cleanup cron. And also enable to use property placeholder in cleanupCron attribute.

Motivation

I want to disable cleanup cron because ...

  • use case 1) When I have many web servers, I want to enable cleanup-cron on only one server and want to disable cleanup-cron on other servers to reduce DataStore load and connection pool usage.
  • use case 2) When SessionDeletedEvent and SessionExpiredEvent seem not to be needed (also no need to close WebSocket connections associated with the session), I want to disable cleanup-cron entirely.

Limitation of current implementation

As shown in example code of #1119 (comment), current RedisHttpSessionConfiguration and JdbcHttpSessionConfiguration does not formal way to disable cleanup cron.

My understanding is that above example code is a only way currently and such way is poor in maintainability.

What I want

My proposal is to add following attributes to disable cleanup cron:

  • Add boolean enableCleanupCron = true optional attribute into @EnableRedisHttpSession and @EnableJdbcHttpSession
    • Default value is true, it is same with existing behavior
    • When it is false, RedisHttpSessionConfiguration and JdbcHttpSessionConfiguration will not register CronTask
  • Enable to use property placeholder in disableCleanupCron and cleanupCron attributes
    • As described in use case A, sometimes user want to vary cleanup cron setting by environment.
    • Use StringValueResolver to resolve placeholders seems to be a way to resolve placeholders (current implementation uses it on redisNamespace attribute but not on cleanupCron).
    • Current cleanupCron attribute seems to accept only cron form (e.g. 0 0 5 31 2). Thus introducing StringValueResolver is not breaking change in my understanding.
@saiya
Copy link
Author

saiya commented Aug 17, 2018

I will try to make a pull request in a way as described above.
If someone have a opinion, please tell me.

@vpavic vpavic added this to the 2.1.0.M3 milestone Aug 17, 2018
@vpavic
Copy link
Contributor

vpavic commented Aug 17, 2018

Rather than adding a new annotation attribute, we could look into direction of introducing a special value for current cleanupCron attribute that would disable it. Another benefit of that approach is that there is no additional work on Spring Boot to support new attribute via configuration property.

saiya pushed a commit to saiya/spring-session that referenced this issue Aug 18, 2018
saiya pushed a commit to saiya/spring-session that referenced this issue Aug 18, 2018
saiya added a commit to saiya/spring-session that referenced this issue Aug 18, 2018
saiya added a commit to saiya/spring-session that referenced this issue Aug 18, 2018
@vpavic
Copy link
Contributor

vpavic commented Aug 18, 2018

Closing in favor of PR #1160.

@vpavic vpavic closed this as completed Aug 18, 2018
@vpavic vpavic added status: duplicate A duplicate of another issue and removed Data Store in: redis type: enhancement A general enhancement labels Aug 18, 2018
@vpavic vpavic removed this from the 2.1.0.M3 milestone Aug 18, 2018
@vpavic vpavic self-assigned this Aug 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants