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

Add support in cache:create() for 'expireAfterWrite' in $config #4975

Conversation

nverwer
Copy link

@nverwer nverwer commented Jul 10, 2023

Description:

This feature adds a configuration option 'expireAfterWrite' to the $config parameter of cache:create().
This option is available in the Caffeine cache, but was not supported in eXist.

Reference:

I have not made a GitHub issue requesting this feature.

Type of tests:

A test has been added to extensions\modules\cache\src\test\xquery\modules\cache\cache.xqm.
It is similar to the test for expireAfterAccess.

@joewiz
Copy link
Member

joewiz commented Jul 10, 2023

@nverwer Thank you for this PR! I see it's opened against the develop-6.x.x branch. I'm curious, do you plan to port it to develop so it's available for eXist 7.x and beyond?

@PieterLamers
Copy link

PieterLamers commented Jul 10, 2023

Thanks @nverwer ! This has been a long-standing request from our side, but which never made it to the issue list.

@nverwer
Copy link
Author

nverwer commented Jul 10, 2023

@joewiz I could also put this in develop, but I would first have to get compilation working in Java 17.

@joewiz
Copy link
Member

joewiz commented Jul 10, 2023

@nverwer If it's convenient, could you please rebase now that #4974 has been merged?

Copy link
Member

@dizzzz dizzzz left a comment

Choose a reason for hiding this comment

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

ok

@nverwer nverwer force-pushed the feature/6.x.x-cache-expireAfterWrite branch from a97bda0 to 49f97ce Compare July 10, 2023 19:15
@nverwer
Copy link
Author

nverwer commented Jul 10, 2023

@joewiz I have rebased onto #4974 and force pushed. No news on java17 / 7.x.x yet.

Some tests fail, but it is not clear to me if that's a problem with my code.

@adamretter adamretter self-requested a review July 11, 2023 09:16
Copy link
Member

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

There is a small issue in the documentation of the new function parameter. Once that is resolved, this is good to go. Thanks @nverwer

@@ -62,7 +62,7 @@ public class CacheFunctions extends BasicFunction {
"Explicitly create a cache with a specific configuration",
returns(Type.BOOLEAN, "true if the cache was created, false if the cache already exists"),
FS_PARAM_CACHE_NAME,
param("config", Type.MAP, "A map with configuration for the cache. At present cache LRU and permission groups may be specified, for operations on the cache. `maximumSize` is optional and specifies the maximum number of entries. `expireAfterAccess` is optional and specifies the expiry period for infrequently accessed entries (in milliseconds). If a permission group is not specified for an operation, then permissions are not checked for that operation. Should have the format: map { \"maximumSize\": 1000, \"expireAfterAccess\": 120000, \"permissions\": map { \"put-group\": \"group1\", \"get-group\": \"group2\", \"remove-group\": \"group3\", \"clear-group\": \"group4\"} }")
param("config", Type.MAP, "A map with configuration for the cache. At present cache LRU and permission groups may be specified, for operations on the cache. `maximumSize` is optional and specifies the maximum number of entries. `expireAfterAccess` is optional and specifies the expiry period for infrequently accessed entries (in milliseconds). `expireAfterAccess` is optional and specifies the expiry period after the entry's creation, or the most recent replacement of its value (in milliseconds). If a permission group is not specified for an operation, then permissions are not checked for that operation. Should have the format: map { \"maximumSize\": 1000, \"expireAfterAccess\": 120000, \"permissions\": map { \"put-group\": \"group1\", \"get-group\": \"group2\", \"remove-group\": \"group3\", \"clear-group\": \"group4\"} }")
Copy link
Member

Choose a reason for hiding this comment

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

There are two entries here for expireAfterAccess!
Presumably the second should instead be expireAfterWrite?

Additionally, an example for expireAfterWrite should be added to the Should have the format: map example.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @adamretter for spotting this. I have made changes accordingly.

@nverwer
Copy link
Author

nverwer commented Jul 11, 2023

I have made a new pull request #4977 for the develop branch (eXist 7.x.x).

@adamretter adamretter force-pushed the feature/6.x.x-cache-expireAfterWrite branch from ac1d498 to a56bdec Compare July 11, 2023 19:20
@adamretter adamretter force-pushed the feature/6.x.x-cache-expireAfterWrite branch from a56bdec to 76a633e Compare July 11, 2023 19:33
@adamretter
Copy link
Member

I have made a new pull request #4977 for the develop branch (eXist 7.x.x).

@nverwer Thanks :-) As your other PR only has one commit, I squashed your two commits in this PR into one - so that they are equivalent.

@adamretter adamretter changed the title add support in cache:create() for 'expireAfterWrite' in $config Add support in cache:create() for 'expireAfterWrite' in $config Jul 11, 2023
@adamretter adamretter self-requested a review July 11, 2023 19:37
Copy link
Member

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @nverwer

@adamretter adamretter merged commit ee55702 into eXist-db:develop-6.x.x Jul 11, 2023
4 of 14 checks passed
adamretter pushed a commit to nverwer/exist that referenced this pull request Jul 11, 2023
@sonarcloud
Copy link

sonarcloud bot commented Jul 11, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 2 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

78.3% 78.3% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@joewiz joewiz added this to the eXist-6.2.1 milestone Nov 10, 2023
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.

6 participants