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

Enable access to Cache Delegates #572

Open
FlorianSchaetz opened this issue Feb 9, 2016 · 14 comments
Open

Enable access to Cache Delegates #572

FlorianSchaetz opened this issue Feb 9, 2016 · 14 comments
Labels
enhancement Improve a feature or add a new feature waiting for feedback

Comments

@FlorianSchaetz
Copy link
Contributor

When using a Cache, especially when using custom Cache implementations, it can be desirable to access that Cache object. Unfortunately, most of the time it will be wrapped into a LoggingCache or similar, which hides this implementation. If would be helpful if LoggingCache, etc. offered a method to get their delegate, so that access to the real Cache instances would be possible. While it is true that direct access to an underlying object might break behaviour of the Cache chain, without such an access, custom Caches will have to be registered into a custom HashMap or similar which tends also be error-prone, especially in a concurrent environment.

@terba
Copy link

terba commented Aug 26, 2016

I would also use this feature on the default cache implementation to access the LoggingCache decorator to get its statistical data. Currently the only accessible statistical value of a cache is its size. Hit ratio is only available in the logs, but I would like to publish that info too on our app's API.

If you are there, please create also public getters for hits, requests and hitRatio attributes as well. Thanks in advance!

@harawata harawata added the enhancement Improve a feature or add a new feature label Sep 13, 2016
@wuwen5
Copy link
Contributor

wuwen5 commented Nov 11, 2016

While it is true that direct access to an underlying object might break behaviour of the Cache chain, without such an access, custom Caches will have to be registered into a custom HashMap or similar which tends also be error-prone, especially in a concurrent environment.

If custom cache can extends from default cache(PerpetualCache), retain the default cache decorators, this is also possible.

like this #818
@harawata Do you agree with this process?if ok, I reopen this

@harawata
Copy link
Member

@wuwen5
Hi,
Requiring a custom cache to extend PerpetualCache seems to be a big constraint.
I have just started looking into cache related issues, so please give me some time to figure out a way to achieve what you guys want.

@harawata
Copy link
Member

harawata commented Nov 16, 2016

@FlorianSchaetz ,

You wrote...

most of the time it will be wrapped into a LoggingCache or similar

What did you mean by "or similar" here?
I can see a custom cache is wrapped by LoggingCache, but don't see 'similar' cases.

@terba @wuwen5 ,

To log statistics, isn't it better to use the statistics provided by each custom cache implementation like below?

If so, just stop wrapping custom caches in LoggingCache could be a solution.

p.s.
By 'custom caches', I meant the external 3rd party cache implementations and not the internal ones.

@terba
Copy link

terba commented Nov 17, 2016

@harawata: I would use that feature with the default (myBatis internal) cache implementation.

@harawata
Copy link
Member

@terba Yes, you wrote that in your previous comment. Sorry I had missed it!

I am wondering if it's possible to satisfy @FlorianSchaetz 's request (i.e. accessing external custom cache implementation) without exposing cache delegates.
It wouldn't solve your issue, but one problem at a time. :)

@wuwen5
Copy link
Contributor

wuwen5 commented Nov 18, 2016

Hi @harawata , I have an idea, we can create an abstract cache decorator class.
like this #838, Can you agree this way?

@Test
  public void getSpecifiedDecorator() {
    Cache cache = new PerpetualCache("default");
    cache = new LruCache(cache);
    cache = new LoggingCache(cache);
    cache = new SynchronizedCache(cache);

    cache.putObject("hello", System.currentTimeMillis());
    cache.getObject("hello");

    LoggingCache loggingCache = findCacheDecorator((CacheDecorator)cache, LoggingCache.class);

    assertNotNull(loggingCache);
    assertEquals(1,loggingCache.getHits());

    PerpetualCache cacheDecorator = findCacheDecorator((CacheDecorator) cache, PerpetualCache.class);

    assertNotNull(cacheDecorator);
  }

  private <T> T findCacheDecorator(CacheDecorator cache, Class<T> type) {
    Cache delegate = cache.getDelegate();
    if (delegate.getClass().equals(type)) {
      return (T) delegate;
    } else if (delegate instanceof  CacheDecorator) {
      return findCacheDecorator((CacheDecorator)delegate, type);
    }
    return null;
  }

@wuwen5
Copy link
Contributor

wuwen5 commented Nov 18, 2016

and this eliminates duplicate code for the cache decorator implements.

@harawata
Copy link
Member

Hi @wuwen5 ,

Thank you for the PR, but I don't think allowing access to the delegates is a good idea.
For example, SynchronizedCache is useless if your code accesses the underlying delegate.
That is why I am trying to solve this issue ( @FlorianSchaetz 's issue) in a different way.

Regarding @terba 's request, it may be useful to provide basic statistics like hit ratio when using the built-in caches, but providing it via LoggingCache does not seem to be right approach, in my opinion.

@daitangio
Copy link

I have similar trouble: I need to get some more stats like max cache size during its life, and I cannot just extend PersistentCache. there is a way to provide a Custom decorator to the CacheBuilder or to extend it a bit?
Also why it is not possible to apply decrators on top of custom caches? I mean...this code inside Cahce Builder

  public Cache build() {
    setDefaultImplementations();
    Cache cache = newBaseCacheInstance(implementation, id);
    setCacheProperties(cache);
    // issue #352, do not apply decorators to custom caches
    if (PerpetualCache.class.equals(cache.getClass())) {
      for (Class<? extends Cache> decorator : decorators) {
        cache = newCacheDecoratorInstance(decorator, cache);
        setCacheProperties(cache);
      }
      cache = setStandardDecorators(cache);
    } else if (!LoggingCache.class.isAssignableFrom(cache.getClass())) {
      cache = new LoggingCache(cache);
    }
    return cache;
  }

cui prodest? why we avoid decorators for custom caches, but we support LoggingCache?

@harawata
Copy link
Member

harawata commented Dec 21, 2022

@daitangio
Could you elaborate on your requirements?

  • Which cache implementation do you use?
  • Which class do you need to access to get the 'stats'?

@daitangio
Copy link

For sure. I need a way to access to all the caches to collect stats about their usage (for instance how much % is used and so on).
I need to do maths on these stats.

The only way I find out was to define a custom cache: but doing so I loose all the standard decorators (like LRU + ability to flush them and so on) and it was a disaster :) See "// issue #352, do not apply decorators to custom caches" above.

It could be nice to extend the LoggingCache class if possible or to be able to decorate a custom perpetual cache... OR there is a simple way to access all the PerpetualCache's instances?
Thank you for your prompt response!

@harawata
Copy link
Member

harawata commented Dec 21, 2022

Thank you for a reply, @daitangio !

I am thinking about adding unwrap(Class) method to org.apache.ibatis.cache.Cache (like JCache's Cache interface).
Basically, by calling cache.unwrap(PerpetualCache.class), you can get the PerpetualCache instance.
Does this solve your problem?
(I would appreciate inputs from other commenters as well!)

As it involves other modification and will be a backward incompatible change, you may have to wait for the next major (minor?) version which is under discussion currently.

p.s.
I know this contradicts my earlier comments about allowing access to the delegate and I still have concerns, but it could be fine if it's used properly...

@daitangio
Copy link

Hi, I only need two things:

  1. a static method to access all the caches like
    List<org.apache.ibatis.cache.Cache> getAllActiveCaches()
    because the cache interface already expose getSize().

  2. a way to understand the current cache capacity, like
    float getCapacity()
    with a value between 0 and 1 (=100% capacity).

The (2) is not mandatory because you can "discover" the underlying implementation easily, but the first one is needed and as far as I know, there is no way to obtain it (but I may be wrong).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve a feature or add a new feature waiting for feedback
Projects
None yet
Development

No branches or pull requests

6 participants