5 Replies Latest reply on Jul 17, 2007 6:04 PM by Galder Zamarreño

    Consolidate delegating cache loader classes [JBCACHE-1134]

    Galder Zamarreño Master

      This thread is to discuss http://jira.jboss.com/jira/browse/JBCACHE-1134.

      Here's a summary of my thoughts taken from JIRA:

      In the org.jboss.cache.loader package, we have:
      o.j.c.l.DelegatingCacheLoader and
      o.j.c.l.AbstractDelegatingCacheLoader

      The second one was created by me when I first created SingletonStoreCL (doh!). I refactored common code from AsyncCL to AbstractDelegatingCL and made AsyncCL and SingletonStoreCL extend it.

      I have just spotted DelegatingCacheLoader which is extended by LocalDelegatingCacheLoader and TcpDelegatingCacheLoader.

      The difference between the classes extending DelegatingCacheLoader and the classes extending AbstractDelegatingCacheLoader is:

      Classes extending DelegatingCacheLoader (LocalDelegatingCacheLoader/TcpDelegatingCacheLoader) are standalone classloaders that delegate to other cache instances. ClusteredCacheLoader could be defined as a refined delegating cache loader in comparison to LDCL and TDCL, in the sense that they delegate get() calls, but don't delegate put() calls because replication takes care of it.

      Classes extending AbstractDelegatingCacheLoader are decorators of other
      class loaders that delegate to cache loaders instead of other caches. As such, AbstractDelegatingCacheLoader adds getCacheLoader()/setCacheLoader() methods that help the interaction between the decorator cache loader and the real cache loader.

      So, this is what I'm proposing:
      - two interfaces, one for CLs that delegate to caches and one for CLs that delegate to CLs.
      - provide abstract implementations for these interfaces with common functionality.
      - refactor CL implementations to adhere to this hierarchy. This should include making ClusteredCacheLoader implement the interface or extend the abstract implementation, of a CL that delegates to another cache taking in account that it only delegates get() operations.

      Thoughts?

      p.s. interfaces need a bit more baking to define the names and the contract...etc.

        • 1. Re: Consolidate delegating cache loader classes [JBCACHE-113
          Manik Surtani Master

          If you look at the DelegatingCacheLoader, it doesn't really do anything at all except proxy calls to abstract methods that need implementing - e.g., put() -> delegatePut(), get() -> delegateGet(), etc etc. Why not just have the implementation (E.g., TcpDelegatingCacheLoader) just implement CacheLoader directly rather than extend DelegatingCacheLoader (the way CCL does)? You have to implement the same number of methods anyway.

          The only thing DCL adds is tracking transactions in a map. This is also done in AbstractCacheLoader, which DCL ought to extend.

          Now if DCL provided no-op impls of the delegateXYZ() methods, then this makes sense since the implementation class could only override calls it would like to delegate.

          In this case I see a benefit for CCL extending DCL.

          AbstractDelegatingCacheLoader (which delegates to other cache loaders) - makes more sense in it's implementation, although I don't quite see much to be consolidated between ADCL and DCL.

          What does need refactoring though is the names! AbstractDelegatingCacheLoader (delegates to cache loaders) and DelegatingCacheLoader (delegates to caches)? Hugely confusing. :-)

          • 2. Re: Consolidate delegating cache loader classes [JBCACHE-113
            Galder Zamarreño Master

            I wanted to note one thing that I forgot in my initial explanation. The reason that lead me to look at all this was the fact that I need any SingletonStoreCacheLoader implementations to extend AbstractDelegatingCacheLoader. I would rather customer having to implement an interface rather than extend a class as it gives them more flexibility. They can always extend AbstractDelegatingCacheLoader and gain from the benefits of overriding just the methods they want, but if alternatively they can implement an interface with getCL/setCL (a name that implies a CL that delegates to another CL, with the possibility of getting/setting the underlying CL), much better :)

            I totally agree with the naming being confusing, hence my attempt to make a separation between cache/CL delegating CLs.

            I think TcpDelegatingCacheLoader is a bit different to the rest of cache delegating caches.

            The rest, LDCL, CCL, and the possible outcome of http://jira.jboss.com/jira/browse/JBCACHE-789, do pretty much the same, delegate cache loading to other caches. First one to a local cache, second to another cache via MethodCalls (using the caches RPC manager) and the third one via proxies. All three just mentioned could just extend AbstractCacheLoader as you said and override what needs overriding.

            I like this idea (which is what u're saying in your 1st paragraph) more than having CCL extend DCL.

            AbstractDelegatingCacheLoader (which delegates to other cache loaders) - makes more sense in it's implementation


            Of course it does!!, i created it myself! hehehehe :-p ;-). This is the cache loader that I want getCL/setCL factored out into an interface to make people who re implement SSCL easier as mentioned earlier.

            • 3. Re: Consolidate delegating cache loader classes [JBCACHE-113
              Galder Zamarreño Master

              Quick note on the requirement of SSCL to have getCL/setCL. getCL is necessary so that SSCL delegates to that CL. setCL is there so that when processing the configuration, we can set the CL that it needs to delegate to. In the same fashion that async cl does.

              • 4. Re: Consolidate delegating cache loader classes [JBCACHE-113
                Manik Surtani Master

                Ok, here is what I propose:

                1) Remove the DelegatingCacheLoader class. I don't see any value add here. TcpDelegatingCacheLoader and LocalDelegatingCacheLoader directly extend AbstractCacheLoader.

                2) Add protected methods getCacheLoader()/setCacheLoader() to AbstractDelegatingCacheLoader so all implementors need to do - at a minimum - is to extend the class and call setCacheLoader() in the constructor to set who to delegate to.

                WDYT?

                • 5. Re: Consolidate delegating cache loader classes [JBCACHE-113
                  Galder Zamarreño Master

                  Re 1 - OK --> this needed doing to avoid confusion.
                  Re 2 - setCL/getCL are already in ADCL, so I'm happy to leave them either public or protected. Protected might make more sense cos only o.c.j.loader code or subclasses should be using it.