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

Default spec not used when specific cache spec is not defined #88

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rmewanou
Copy link

@rmewanou rmewanou commented May 27, 2021

Thanks for creating this library, @stepio. We're leveraging and loving it very much.

This change is to fix a situation where a specific cache name spec is not defined and the default spec string is not being used for that cache name. We came across that issue when running it in production and our cache size was growing indefinitely.

@stepio
Copy link
Owner

stepio commented Jun 13, 2021

Hello @rmewanou , nice to e-meet you!

Thanks a lot for enjoying the library and coming back with the suggestion.

If understand your use case correctly, when you don't specify value for coffee-boots.cache.spec.<your cool cache here>, you expect that coffee-boots.cache.basic-spec (sorry for confusing error in README) acts as a default configuration.

However my idea was that the library extends the Spring built-in functionality, not replaces it, so I expected that if you need such a default configuration, you just specify spring.cache.caffeine.spec instead.

The custom property coffee-boots.cache.basic-spec is intended to fulfill a niche use case when you want to make sure that specific configuration option is used for all custom caches (unless overridden, of course), e.g.: coffee-boots.cache.basic-spec=recordStats.

What do you think? Have you tried specifying spring.cache.caffeine.spec ?

@stepio
Copy link
Owner

stepio commented Jun 14, 2021

Hi again, @rmewanou !

So I tried my suggestion with spring.cache.caffeine.spec and... it does not work. My bad! Apparently it's a side-effect of the major rewrite I did before.

So your change does not make this situation worse. However there is another minor problem if we fix the issue the suggested way - in CaffeineSupplier#apply. Basically the configurations defined programmatically via cacheBuilders are never used if coffee-boots.cache.basic-spec defined.

What if I (or you) add the logic from the previous comment myself and use it as a third option in CaffeineSupplier#apply? Basically if all the existing options (custom properties & programmatically defined caches) exhausted and still no config found, just read the spring.cache.caffeine.spec property and build the appropriate cache.

What do you think?

P.S.: Sorry, closed & reopened the PR accidentally - hot keys made fun of me.

P.P.S.: Ignore the failing builds - http://nist.gov is down for some reason (temporarily, I assume), which breaks OWASP dependency check plugin. This happens few times a year and I don't own (and don't need) any caching proxy to prevent such infrequent issues.

@stepio stepio closed this Jun 14, 2021
@stepio stepio reopened this Jun 14, 2021
@rmewanou
Copy link
Author

rmewanou commented Jul 5, 2021

Hey @stepio, thanks for confirming the issue and explaining your initial thoughts on that. Please go ahead and apply the necessary changes in a new PR if you want to wrap this up. I was mostly away from public github for few days and missed your reply, apologies.

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

Successfully merging this pull request may close these issues.

None yet

2 participants