Skip to content

Add support for cache2k in memory caching #28498

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

Closed
wants to merge 1 commit into from

Conversation

cruftex
Copy link
Contributor

@cruftex cruftex commented Nov 1, 2021

cache2k is a highly efficient in memory cache with low memory overhead and high throughput.
Cache2k outperforms all other caches by a factor of two or more for cache friendly workloads,
see the latest benchmarks at: https://cache2k.org/benchmarks.html.

Cache2k has a bunch of features not available in other caches, that help to build
low latency and resilient applications, e.g. refresh ahead, resilience and exception support.

There is a well defined Java caching API (see: https://cache2k.org/docs/latest/apidocs/cache2k-api/index.html) as well as an extensive user guide (see: https://cache2k.org/docs/latest/user-guide.html).

Additional goodies are support for JSR107/JCache, JMX statistics and management like online cache size tuning,
Spring Framework integration and micrometer statistics export.

With 400kB cache2k is one of the most compact and feature rich caching libraries. It supports pure
Java 8 without making use of sun.misc.Unsafe. With the next version of cache2k it is planed to
require Java 11 and use its advanced features to improve the performance and efficiency even more.

Spring framework support in cache2k is available for a while. Comparing to Caffeine it supports
configuration of individual caches out of the box, see:
https://cache2k.org/docs/latest/user-guide.html#spring

This pull request adds Spring Boot support for cache2k.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 1, 2021
@snicoll snicoll added the for: team-meeting An issue we'd like to discuss as a team to make progress label Nov 2, 2021
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I've flagged this one to discuss if we want to bring support for cache2k in Spring Boot. Feel free to ignore the comments below for now.

@@ -39,6 +39,9 @@ dependencies {
optional("com.datastax.oss:java-driver-core")
optional("com.fasterxml.jackson.dataformat:jackson-dataformat-xml")
optional("com.github.ben-manes.caffeine:caffeine")
optional("org.cache2k:cache2k-api")
Copy link
Member

Choose a reason for hiding this comment

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

I'd expect the API to be added from either the spring or micrometer dep.

@@ -44,6 +47,17 @@
@ConditionalOnClass(MeterBinder.class)
class CacheMeterBinderProvidersConfiguration {

@Configuration(proxyBeanMethods = false)
@ConditionalOnClass({ SpringCache2kCache.class, Cache2kBuilder.class, org.cache2k.extra.micrometer.Cache2kCacheMetrics.class })
Copy link
Member

Choose a reason for hiding this comment

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

Cache2kCacheMetrics doesn't need to be fully qualified unless I am missing something.

@@ -14,6 +14,9 @@ dependencies {
optional("com.datastax.oss:java-driver-core")
optional("com.fasterxml.jackson.core:jackson-databind")
optional("com.fasterxml.jackson.datatype:jackson-datatype-jsr310")
optional("org.cache2k:cache2k-api")
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

@@ -65,6 +65,9 @@ dependencies {
optional("org.apache.tomcat:tomcat-jdbc")
optional("org.apiguardian:apiguardian-api")
optional("org.codehaus.groovy:groovy-templates")
optional("org.cache2k:cache2k-api")
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

public static class Cache2k {

/**
* Alternative cache2k cache manager name to use.
Copy link
Member

Choose a reason for hiding this comment

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

"Alternative" sounds a bit odd here. What is the purpose of this name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unlike Guava or Caffeine, caches in cache2k are managed. That means caches have a name and live within a cache manager. This allows caches to have external configuration or monitoring exports. When working on a larger code base, it can make sense to separate caches into different managers, e.g. "spring" for the caches used by the Spring cache abstraction, "hibernate" for database caches. The advantage is, besides better overview, that you can apply different default configurations to a group of caches. For example, enable micrometer export for all hibernate caches as well.

When running multiple application containers in parallel, it is also needed to separate the caches into different managers to avoid conflicts.

Putting the manager name in the Spring configuration is basically the same concept as the config resource in JCache. However, a config resource doesn't make sense for cache2k since file based configuration is optional.

I think it is a good idea to put a bit more in the Java doc comment.

@cruftex
Copy link
Contributor Author

cruftex commented Nov 2, 2021

Thanks for the PR. I've flagged this one to discuss if we want to bring support for cache2k in Spring Boot. Feel free to ignore the comments below for now.

Thanks for the swift review and giving it a look!

Spring support is available in cache2k for a while and it is continuously integration tested. cache2k has a consistent and complete user guide that helps users when starting with caching. Looking at EHCache3 or Caffeine users need to puzzle together several blog posts and Stackoverflow answers when starting. I think in that perspective the project philosophy of Spring and cache2k is similar.

NB: The cache2k Spring integration is exploiting the null value support, while most other caches are wrapping the cached value (EHCache3, Caffeine, JCache bases caches). This reduces the memory footprint.

Since it is not mentioned in the Spring documentation, unfortunately it is not very visible for Spring users. I assume mentioning it in the Spring documentation without Spring boot support does not make sense, since the user experience would not be consistent to the other caches.

@philwebb philwebb force-pushed the main branch 3 times, most recently from 1ca278f to 902dd0b Compare November 19, 2021 20:17
@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels Nov 22, 2021
@philwebb philwebb added this to the 2.7.x milestone Nov 22, 2021
@snicoll
Copy link
Member

snicoll commented Nov 29, 2021

@cruftex we've decided to merge this for our next feature release. Our first milestone is next year so there's plenty of time but if you could please have a look to the review in the meantime? Thanks!

@cruftex
Copy link
Contributor Author

cruftex commented Nov 30, 2021

@snicoll Thanks, this is great news! I plan to wrap up the next cache2k version next week and update this PR.

@cruftex
Copy link
Contributor Author

cruftex commented Dec 30, 2021

Updated the PR:

  • Suggested changes from review above
  • CheckStyle issues
  • formatting issues
  • improved JavaDoc

Let me know if I should rebase/squash on my side.

@snicoll
Copy link
Member

snicoll commented Dec 30, 2021

Yes please.

@cruftex
Copy link
Contributor Author

cruftex commented Dec 30, 2021

Ok! Rebased.

@snicoll snicoll self-assigned this Jan 3, 2022
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

I've started to review and polish this so please don't push further update at this point but there is one aspect that is at odds with the other implementation. I've added a suggestion, let me know what you think.

Polished code is here: https://github.com/snicoll/spring-boot/tree/pr/28498

* @param cacheManager the cache manger to add the caches
* @param cacheNames the cache names
*/
private void addListedButUnknownCaches(SpringCache2kCacheManager cacheManager, Collection<String> cacheNames) {
Copy link
Member

@snicoll snicoll Jan 3, 2022

Choose a reason for hiding this comment

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

This is at odd with all the other implementations. Customizers run after cache names have been set. Properties or bean defined in the context are used to customize how caches are created by default. Because cache names are properties-based as well, it is expected to be processed before the customizers run and I am not keen to change that order for Cache2K only.

Cache2K has a defaultSetup function but I don't think exposing that as a bean is a good idea. I am tempted to expose this:

public interface Cache2kBuilderCustomizer {

    void customize(Cache2kBuilder<?, ?> builder);

}

And call defaultSetup with a function that would invoke those. This makes CacheManagerCustomizer less useful but that is already the case for anything that changes the default setup. The name doesn't really imply that it changes the default setup so we may want to rename this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

Checking the other implementations more thoroughly I found another issue: E.g. the implementations Caffeine, JCache, SimpleCacheConfiguration allow no dynamic creation if the list of names is provided. That seems to be a standard and should be added also.

It might also be a good idea to pass the class loader, like in the JCache scenario. Cache2k allows setting the classloader and uses it currently when creating class instances from the external configuration. In the future more serialization/deserialization will happen.

W.r.t. to the default setup: The idea Cache2kBuilderCustomizer does the job simple and well, however: It is introducing another new concept and it is not inherently clear that this is actually the default setup. Providing the configuration bean Cache2kConfig seems to be a better fit. Ideally, this could be simply added to the cache properties. I will double check, whether this works.

private void addListedButUnknownCaches(SpringCache2kCacheManager cacheManager, Collection<String> cacheNames) {
Collection<String> knownCaches = cacheManager.getCacheNames();
if (!CollectionUtils.isEmpty(cacheNames)) {
cacheNames.stream().filter((n) -> !knownCaches.contains(n))
Copy link
Member

Choose a reason for hiding this comment

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

I am not keen to do that either. If a cache name is set we will create it. It won't be relevant anyway once we find a solution for the other issue and this is invoked prior to the customizer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. And I realize it is not good to allow/propagate mixing the configuration styles. It should be either Spring boot default way or specific to the used cache implementation.

snicoll pushed a commit to snicoll/spring-boot that referenced this pull request Jan 3, 2022
snicoll added a commit to snicoll/spring-boot that referenced this pull request Jan 3, 2022
@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Jan 4, 2022
@cruftex
Copy link
Contributor Author

cruftex commented Jan 5, 2022

I have commented the polished pull request. Things I would like to add:

  • Default configuration support, possibly via Cache2kConfig in the properties. I will do some tests for that.
  • Additional tests for dynamic creation and managerName = null
  • maybe class loader support

However, the polished PR is already good enough to integrate. Let me know if you want to merge your polished version and I do a new PR or whether I should continue here after adding the polished commit.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 5, 2022
@snicoll
Copy link
Member

snicoll commented Jan 5, 2022

Thanks for looking at it.

However, the polished PR is already good enough to integrate.

It can't, the build is failing on the test that configures the defaults and expect them to be available by the time the auto-configuration creates a cache for the names defined in the standard cache-names property. My proposal on the customizer is really what's blocking this to be merged. I don't know if that's the right approach. I wasn't aware of Cache2kConfig. Would exposing a bean of that type help configuring default caches? It certainly would be more aligned with what we're doing elsewhere.

Regarding exposing this as properties, I am not keen to do that. We've got several requests to expose a large number of properties for other cache libraries and didn't act on it. We might revisit this at some point but I don't want to do it for one store only.

@snicoll snicoll added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jan 5, 2022
@cruftex
Copy link
Contributor Author

cruftex commented Jan 8, 2022

Cache2kConfig: There is SpringCache2kCacheManager.setCaches(Collection<Cache2kConfig<?, ?>> cacheConfigurationList) already. The default config needs to be copied and then the name set. A proper copy function is present in cache2k-config because its needed within the XML configuration support, however, not exposed at the moment. Spring BeanUtils might work as well. The setCaches method has a bit thin test coverage at the moment, only one very simple. I will make some tests with useful parameters, e.g. expiry and capacity and see whether the BeanUtils are sufficient.

Regarding exposing this as properties, I am not keen to do that.

That is understandable. Things seem to get already messy there, e.g. having an expiry parameter at one implementation and timeToLive at the other, however, that is essentially the same concept. There will be constant flux, if parameters controlling any cache feature are added. A single config parameter like the spec String or the config bean avoids the flux and let both products evolve independently.

OTOH, all other caches, except JCache, allow basic configuration in the properties and Caffeine allows complete configuration, since it has all configuration parameters available via the spec string. JCache is an exception, because you will always need implementation specific parameters and MutableConfiguration fails to provide a simple setting (expiry) as a simple bean property. Also, thinking towards user documentation, a simple setup with reasonable defaults should look similar across cache implementations and or at least not mix different configuration approaches. So with cache2k it should be possible to configure basic parameters in the properties as well, to be consistent. Maybe that is good reasoning. What do you think?

Would it be helpful to have a single string for all parameters, like Caffeine?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 13, 2022
@snicoll
Copy link
Member

snicoll commented Jan 13, 2022

This was included as example

Sorry, I missed the setup method there. My initial comment still stands though. If you want to customize something, you loose the default behavior. Sure there is a applySpringDefaults but you could easily forget to call it (or even not knowing it exists).

IMO this comes back to the default lambda thing. Rather than requiring to implement the interface, I think getting a default callback with the defaults applied and letting you customize it further would be much better.

@cruftex
Copy link
Contributor Author

cruftex commented Jan 14, 2022

My thinking was that it supplier approach is an elegant solution for providing defaults and/or change to another cache manager.
But, yes, that might be irritating or lead to errors. So back to your original idea:

interface DefaultCache2kBuilderCustomizer {
  void customize(Cache2kBuilder<?, ?> builder);
}

I think just Cache2kBuilderCustomizer is too generic.

In case users want to use another CacheManager they can include the SpringCache2kCacheManager in the configuration and bind it to any native cache manager. So actually, if users want to do more complex things, that is covered anyways.

IMHO the DefaultCache2kBuilderCustomizer should live in Spring Boot, since its solely used when Spring Boot is creating the cache manager. Alternatively I could start working on cache2k/cache2k#153 and put this into the cache2k-api, however, I think it is better to keep this separated. Maybe users want to have a native cache2k cache manager in the DI container as well, this needs another default than the cache manager for the Spring cache abstraction.

There is another thing that makes me currently unhappy. Looking through the Caffeine integration I can see that the caches get refreshed when the default is changed and also the method registerCustomCache would overwrite a previously created cache. This means, for Caffeine it is possible to change the defaults in the CacheManagerCustomizer. The refreshing in Caffeine drops the previously created caches and creates new ones. Since cache2k caches are managed and might cause additional events when created, the same cannot be done. To achieve the functionality (being able to adjust defaults and add custom caches in the CacheManagerCustomizer) with cache2k, it makes actually sense to process the cache names after the manager customizer and only add caches with default setup that are not yet present. I think it was similar in my initial PR, now I know why. The same would actually make sense for Caffeine as well because the (ugly) refreshing would not be needed any more.

Since the above problematic only applies if the names property and a CacheManagerCustomizer is specified, another approach could be to simply forbid this combination (for cache2k). That is better than a different semantic.

Let me know what you think. How to proceed?

@snicoll
Copy link
Member

snicoll commented Jan 14, 2022

What you've started with the supplier should stay in one form or another, it is much better than us introducing the customizer. Having a default implementation that applies the defaults and take a customizer should be enough. Or even what you have at the moment if you're happy with it.

it makes actually sense to process the cache names after the manager customizer and only add caches with default setup that are not yet present.

I don't think I agree but it doesn't matter very much as that ship has sailed. We could only really change that order in a major release. For 3.0, we could consider applying the cache names in a customizer with an order of 0 so that you could chose to be called before or after.

There has been quite a lot of chatter so I hope we can come to a conclusion on this soon.

@cruftex
Copy link
Contributor Author

cruftex commented Jan 17, 2022

I don't think I agree but it doesn't matter very much as that ship has sailed. We could only really change that order in a major release. For 3.0, we could consider applying the cache names in a customizer with an order of 0 so that you could chose to be called before or after.

Let's better leave that topic for now. Probably it needs more time (and example code) to discuss this thoroughly, but it is not essential. Maybe I am trying to be too perfect.

There has been quite a lot of chatter so I hope we can come to a conclusion on this soon.

Alright. Updates:

I released version 2.5.2.Alpha and consolidated the spring-boot changes into:
https://github.com/cruftex/spring-boot/tree/cache2k-take2

Changes:

  • SpringCache2kManager.defaultSetup throws IllegalStateException if caches have already been added
    (resetting the default works in Caffine, but not in cache2k)
  • SpringCache2kDefaultCustomizer interface added and support added to Cache2kCacheConfiguration
  • Updated tests

Hopefully we are getting close?

@cruftex
Copy link
Contributor Author

cruftex commented Jan 24, 2022

@snicoll: Ping. In the hope it is supportive, I rebased and squashed everything and started a new PR:
#29536
However, if there is more discussion needed, feel free to close the other PR and continue the discussion here.

Please let me if something still needs more work.

@snicoll
Copy link
Member

snicoll commented Jan 24, 2022

SpringCache2kDefaultCustomizer and SpringCache2kDefaultSupplier sound very generic to me. I guess the customizer was introduced as a trade-off for the double lambda thing? If so, I don't think we're making progress on what was discussed.

We now have three ways to customize things, the "default supplier" that configures defaults, the "default customizer" that customizes default, and our customizer that configures the cache manager. I don't think a supplier and a customizer is warranted as the very purpose of the supplier is to do the work of providing the defaults. At best, the customizer could be an implementation of one supplier but shouldn't be the default behaviour as the two aren't connected.

Having a default implementation that applies the defaults and take a customizer should be enough.

So I didn't mean by that creating yet another first-class concept. Rather a default implementation of the supplier that takes, as an argument, a consumer or something. Nothing more than a convenience really.

To sum up. If you want the cache manager to be configured before cache names are applied, the idiomatic way of doing this in Spring Boot is provide a bean that either represents the default configuration, or can be applied on something that configures the default configuration. I have expressed an opinion on how that should be done in Cache2K which can be ignored though.

There's no need to introduce something in cache2k just for the purpose of this PR.

@cruftex
Copy link
Contributor Author

cruftex commented Jan 25, 2022

I guess the customizer was introduced as a trade-off for the double lambda thing? If so, I don't think we're making progress on what was discussed.

No. I added it because of this comment, from #28498 (comment). The double lambda is appearing in the Spring boot test code only but not in the normal user code. Here the comment:

IMO this comes back to the default lambda thing. Rather than requiring to implement the interface, I think getting a default callback with the defaults applied and letting you customize it further would be much better.

If a user only wants to provide defaults, the customizer seems the better and well understood interface to me.
Also you wrote:

What you've started with the supplier should stay in one form or another, it is much better than us introducing the customizer. Having a default implementation that applies the defaults and take a customizer should be enough.

Better have some code. Here a comparison. With customizer:

		@Bean
		SpringCache2kDefaultCustomizer cache2kDefaultCustomizer() {
			return (builder) -> builder.entryCapacity(4711);
		}

Here with wrapping the customizer or some consumer, if I understood you correctly:

		@Bean
		SpringCache2kDefaultSupplier cache2kDefaults() {
			return SpringCache2kDefaultSupplier.of((builder) -> builder.entryCapacity(4711));
		}

Feels rather ugly for supplying the defaults. But maybe you had a Spring trick in mind that I don't know of?

N.B.: One of the complicating factor here is the native null support, which is boggling my mind. Other caches don't have that problem and simply can work with supplying a complete cache configuration that does not need to have a Spring specific default.

We now have three ways to customize things, the "default supplier" that configures defaults, the "default customizer" that customizes default, and our customizer that configures the cache manager. I don't think a supplier and a customizer is warranted as the very purpose of the supplier is to do the work of providing the defaults.

The default supplier still has the use of supplying the manager to be used. Alternatively a ManagerSupplier would be more clear, or, we use the property again, or, we simply leave it out, because its always possible to create the cache manager without auto configuration.

Caffeine has 4 possibilities to provide defaults when cache names are configured: in the properties, providing a builder (Caffeine), providing a CaffeineSpec and then you can use the CacheManagerCustomizer and set defaults again via builder, spec or String. So with the idea or requirement, that I started with, that it should be similar to what people know from other caches, it automatically leads to many different concepts to provide options.

So, yes, better have less different concepts. At the moment I tend to drop the supplier but keep the customizer, because this would be used more frequently and is a known Spring boot concept. Maybe rename it simply to Cache2kDefaults?

For more specialized things the CacheManager could always provided directly. Is there any particular disadvantage in skipping auto configure and providing the CacheManager bean?

Since things got a bit out of hand here and I have the feeling that we are both getting a bit frustrated already, I have the feeling that it could make sense and write down goals and possible setup scenarios to have a more structured discussion. What do you think?

@cruftex
Copy link
Contributor Author

cruftex commented Feb 1, 2022

@snicoll: Cleaned up and simplified the integration.

For the auto configuration defaults there is now the class Cache2kDefaults in Spring boot. Better name ideas are always welcome. The SpringCache2kCacheManager in cache2k also got various improvements to detect probable errors in the configuration, e.g. setting the defaults twice. Although it feels we are back where we started, various details have been improved along the way.

As an orientation I came up with the following matrix:

Auto configuration variants

names in properties Cache2kDefaults CacheManagerCustomizer result
- - - caches created dynamically, upon request via CacheManager.getCache
X - - caches created on startup, metrics registered, no dynamic cache creation allowed
- X - caches created dynamically with specified defaults
X X - caches created on startup with specified defaults, metrics registered, no dynamic cache creation allowed
- X with custom caches and caches with specific configuration added via cache CacheManager, dynamic cache creation with defaults applied allowed
- - with defaults and custom caches caches with defaults and caches with specific configuration added via cache CacheManager
- X with defaults error
X - with defaults error
X X with custom caches named caches with defaults and caches with specific configuration added via cache CacheManager, no dynamic cache creation. This works but is not recommended because the configuration is spread among three different configuration concepts, better set cache names and defaults via the manager customizer

There are tests now for the above variants.

"Manual" configuration

CacheManager created as bean -> enables setting of different cache2k manager name, enables setting of classloader in the future if this is necessary, metrics for known caches get registered.

Looking forward to your comments.

@cruftex
Copy link
Contributor Author

cruftex commented Feb 8, 2022

Bumped cache2k version to 2.6.1.Final
@snicoll: ping!

@cruftex
Copy link
Contributor Author

cruftex commented Feb 11, 2022

Fixed the formatting errors.

@snicoll snicoll removed their assignment Feb 11, 2022
@snicoll snicoll self-assigned this Mar 21, 2022
@snicoll snicoll modified the milestones: 2.7.x, 2.7.0-M3 Mar 21, 2022
snicoll pushed a commit that referenced this pull request Mar 21, 2022
snicoll added a commit that referenced this pull request Mar 21, 2022
@snicoll snicoll closed this in 6805be5 Mar 21, 2022
@snicoll
Copy link
Member

snicoll commented Mar 21, 2022

@cruftex thank you for making your first contribution to Spring Boot.

I think we've been iterating and improving things quite a bit. I don't want to have too many ways of configuring things so I've kept that to dedicated customizer for now. IMO, if you want to prevent defaultSetup to be called twice, it should become a constructor argument of the cache manager.

@cruftex
Copy link
Contributor Author

cruftex commented Mar 24, 2022

@snicoll thanks for adding cache2k!

I think we've been iterating and improving things quite a bit. I don't want to have too many ways of configuring things so I've kept that to dedicated customizer for now.

Yeah, I realized that when I tried to implement "all" the other configuration variants that I found, which led into a terrible mess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants