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

Allow configuration of max size and expiry simultaneously for JCache #194

Merged
merged 1 commit into from
Oct 30, 2017

Conversation

kdombeck
Copy link
Contributor

@kdombeck kdombeck commented Oct 25, 2017

The eager-expiration will not take affect with the following configuration

test-cache {
  policy {
    eager-expiration {
      after-write = 10m
    }

    maximum {
      size = 1000
    }
  }
}

As you can see by this code.

      boolean evicts = configureMaximumSize() || configureMaximumWeight()
          || configureExpireAfterWrite() || configureExpireAfterAccess()
          || configureExpireVariably();

The first configuration that it finds is the only one that gets configured.

I looked at passing mocks into CacheFactory but doesn't seem possible since the configuration gets cloned. So please let me know if there is a better way I should be testing this.

@kdombeck kdombeck changed the title Allow configuration of max size and exiry simultaneously for JCache Allow configuration of max size and expiry simultaneously for JCache Oct 25, 2017
@ben-manes
Copy link
Owner

Wow, I feel pretty foolish.

I think your test looks good. I don’t recall why the configuration is cloned, but it might be for consistency of a different instance, e.g. if reusing the configuration to create multiple caches.

I’ve been swamped and trying to find the time to catch up. I have another fix promised but not sure if I can get to it soon. Is there a timeframe you need this released by?

@kdombeck
Copy link
Contributor Author

No timeframe on my side. Thank you for being so responsive with issues. Truly you are the most responsive of any open source project I have ever worked on.

Thanks again.

@ben-manes
Copy link
Owner

Great. I'll try to catch up over the weekend, so I'll leave this open to prod me.

If you have any concerns or improvements on the JCache implementation, please let me know. Admittedly it doesn't get as much attention. There are upcoming changes in JCache 1.1 and I have the revised TCK mostly passing locally. I also plan on an overhaul for 3.0, when I get the time to start on it, so we could make braking changes if you have suggestions.

@kdombeck
Copy link
Contributor Author

The only thing I can think of is enabling the native Caffeine statistics. The stats in Caffeine are a lot better than the ones exposed in JCache.

@ben-manes ben-manes merged commit 78be027 into ben-manes:master Oct 30, 2017
@ben-manes
Copy link
Owner

Released. Thanks again for PR!

@kdombeck kdombeck deleted the jcache-config branch November 1, 2017 15:56
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.

2 participants