enonic / lib-cache

Cache Library for Enonic XP.
Apache License 2.0
2 stars 1 forks source link

Restructure tests and implementation of cache.getIfPresent and cache.put #40

Closed Hoier closed 1 year ago

Hoier commented 1 year ago

Im working on a project where lib-cache is a key component. I have a need for the com.google.common.cache.Cache put and getIfPresent functions, and implemented those. I also upgraded gradle wrapper to 7.5. Could be usefull for others, hence the pull request. The tests from the previous release was giving mockito warnings in gradle 7.5, so I looked at lib-cron to see how a newly updated lib handled those, and changed lib-cache to test in the same way.

alansemenov commented 1 year ago

@Hoier thanks for the suggested fix, we'll look into it.

as far as upgrade of Gradle wrapper goes, it's not enough to just change version in the properties file, you have to update the binaries as well. see here for correct upgrade command.

Hoier commented 1 year ago

Just realized this solves one issue on this project: https://github.com/enonic/lib-cache/issues/10

Hoier commented 1 year ago

@Hoier thanks for the suggested fix, we'll look into it.

as far as upgrade of Gradle wrapper goes, it's not enough to just change version in the properties file, you have to update the binaries as well. see here for correct upgrade command.

I did the change before running it first time, and have the 7.5 bin in my .gradle folder. I change the properties file back to 6.9, ran the upgrade command, tried to commit but branch is allready up to date with master. Should be good

rymsha commented 1 year ago

What is the use case for those two new methods? What exactly you cannot solve with existing get ?

Hoier commented 1 year ago

@rymsha The issue is not "not beeing able to solve things with get", its more about making cleaner code, and avoiding unnecessary calls to the DB.

When updating a node through apis, I could update the cache directly with put, instead of storing the node, invalidate and then using get. This will also make it possible to update the cache at the same time as the DB.

When Starting up an application, I want to pre-heat certain keys, and will get those in one query from nodeLib.query, instead of getting each spessific node with cache.get(), I could just loop through my array, setting each with put without invalidating and requesting info from the DB again. Queries in Enonic XP are super fast, but stacking up 1000 requests to get each invidually would cause higher CPU load. I know we could prepare a loop where we invalidate each key, doing a get that just returns the array[i], but I would much rather update them direcly.

Using getIfPresent makes it easy to debug, and make it easy for us to create a dashboard with all cached values, and see which keys are not set. If we were to use get on all keys, they would be automatically set.

Its a non-breaking, QOL update to the lib. Using put is an important part of caching strategies like "write through". Using getIfPresent would make it possible for us to visualize what keys are not set, without setting them.

Let me know if anything is unclear or if you have any other question👍

rymsha commented 1 year ago

For all new functionality we require docs to be written. For lib-cache docs are located here: https://github.com/enonic/lib-cache/blob/master/docs/index.adoc Otherwise your PR looks fine.

sigdestad commented 1 year ago

FYI, lib-cache is a per instance API, meaning if you run a cluster each node will have to populate and maintain its own cache.

Did you see the new Grid library, whixh works cluster wide?

Hoier commented 1 year ago

@rymsha Docs updated 👍 @sigdestad I havent, but I definitly will!

Hoier commented 1 year ago

@rymsha All changes commited 👍