I put the abstraction - somewhat hairy as it is, and in need of documentation, it doesn't change the factories at all - in as-is because I found myself copy-and-pasting Basic[GadgetSpec/MessageBundle]Factory's caching logic yet again, which itself seemed prone to errors. I'm not sure I agree that the primary issue with these classes is a duplicated network fetch, at least inasmuch as code duplication is concerned. That's what these classes *do* - fetch, build object. Further, my intent was (and I have this working in a CL right now) to utilize this same caching logic for rewritten content. That operation requires no network fetch at all, so consolidating that logic didn't make sense.
Still, ultimately this base class is an implementation detail that I'm willing to change. The only commonality lending itself to composition that I saw was in wrapping the Cache instance in a Ttl-aware cache. I'm doing that now, a change which I need to do only in CachingWebRetrievalFactory. Once that's done, we may well discover that the amount of logic left in CachingWebRetrievalFactory is small enough that we can remove it and just bite the small bullet that the core retrieval logic for the Factories will be duplicated (if ignoreCache - retrieve - else lookup - if found, cache, etc...) --John On Wed, Aug 27, 2008 at 9:41 PM, Kevin Brown <[EMAIL PROTECTED]> wrote: > I have to disagree on the abstraction entirely here. It's just not > appropriate for what we're trying to achieve. > > This is exactly the sort of thing that should be done through composition, > not inheritance. > > This doesn't eliminate the primary issue with these classes -- the > duplicated network fetches. BasicMessageBundleCache is still doing all the > same work that it was doing before, except for caching. > > This isn't really buying us anything, and it makes the code far more > complicated than it was before. Having duplicate work sucks, but trying to > understand a class that extends some generic that takes 3 parameters (with > no documentation) of which one is duplicated is truly bizarre. > > I think we should just roll this back to what it was. There are far more > important items to address than this minor annoyance. > > On Wed, Aug 27, 2008 at 6:16 PM, <[EMAIL PROTECTED]> wrote: > > > Author: johnh > > Date: Wed Aug 27 18:16:19 2008 > > New Revision: 689691 > > > > URL: http://svn.apache.org/viewvc?rev=689691&view=rev > > Log: > > Slight clean-up of recently-submitted refactoring of > > CachingWebRetrievalFactory. The name is already > > a bad one, but the method signatures can be cleaned up a bit, and are. > > fetchFromWeb is now retrieveRawObject, > > and the cache key type is templatized. > > > > > > Modified: > > > > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractMessageBundleFactory.java > > > > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/BasicGadgetSpecFactory.java > > > > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/BasicMessageBundleFactory.java > > > > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/CachingWebRetrievalFactory.java > > > > Modified: > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractMessageBundleFactory.java > > URL: > > > http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractMessageBundleFactory.java?rev=689691&r1=689690&r2=689691&view=diff > > > > > ============================================================================== > > --- > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractMessageBundleFactory.java > > (original) > > +++ > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractMessageBundleFactory.java > > Wed Aug 27 18:16:19 2008 > > @@ -30,7 +30,7 @@ > > * delegates caching and network retrieval to concreate implementations. > > */ > > public abstract class AbstractMessageBundleFactory > > - extends CachingWebRetrievalFactory<MessageBundle, LocaleSpec> > > + extends CachingWebRetrievalFactory<MessageBundle, LocaleSpec, URI> > > implements MessageBundleFactory { > > > > protected AbstractMessageBundleFactory(CacheProvider cacheProvider, > > > > Modified: > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/BasicGadgetSpecFactory.java > > URL: > > > http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/BasicGadgetSpecFactory.java?rev=689691&r1=689690&r2=689691&view=diff > > > > > ============================================================================== > > --- > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/BasicGadgetSpecFactory.java > > (original) > > +++ > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/BasicGadgetSpecFactory.java > > Wed Aug 27 18:16:19 2008 > > @@ -37,13 +37,15 @@ > > import java.util.List; > > import java.util.concurrent.CountDownLatch; > > import java.util.concurrent.ExecutorService; > > +import java.util.logging.Logger; > > > > /** > > * Basic implementation of a gadget spec factory. > > */ > > @Singleton > > -public class BasicGadgetSpecFactory extends > > CachingWebRetrievalFactory<GadgetSpec, URI> > > +public class BasicGadgetSpecFactory extends > > CachingWebRetrievalFactory<GadgetSpec, URI, URI> > > implements GadgetSpecFactory { > > + static final Logger logger = > > Logger.getLogger(BasicGadgetSpecFactory.class.getName()); > > > > private final HttpFetcher fetcher; > > private final ContentRewriterRegistry rewriterRegistry; > > @@ -53,6 +55,11 @@ > > protected URI getCacheKeyFromQueryObj(URI queryObj) { > > return queryObj; > > } > > + > > + @Override > > + protected Logger getLogger() { > > + return logger; > > + } > > > > public GadgetSpec getGadgetSpec(GadgetContext context) throws > > GadgetException { > > return getGadgetSpec(context.getUrl(), context.getIgnoreCache()); > > @@ -69,7 +76,7 @@ > > * Retrieves a gadget specification from the Internet, processes its > > views and > > * adds it to the cache. > > */ > > - protected FetchedObject<GadgetSpec> fetchFromWeb(URI url, boolean > > ignoreCache) > > + protected FetchedObject<GadgetSpec> retrieveRawObject(URI url, boolean > > ignoreCache) > > throws GadgetException { > > HttpRequest request = new > > HttpRequest(Uri.fromJavaUri(url)).setIgnoreCache(ignoreCache); > > HttpResponse response = fetcher.fetch(request); > > > > Modified: > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/BasicMessageBundleFactory.java > > URL: > > > http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/BasicMessageBundleFactory.java?rev=689691&r1=689690&r2=689691&view=diff > > > > > ============================================================================== > > --- > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/BasicMessageBundleFactory.java > > (original) > > +++ > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/BasicMessageBundleFactory.java > > Wed Aug 27 18:16:19 2008 > > @@ -31,17 +31,18 @@ > > import com.google.inject.name.Named; > > > > import java.net.URI; > > +import java.util.logging.Logger; > > > > /** > > * Basic implementation of a message bundle factory. > > */ > > @Singleton > > public class BasicMessageBundleFactory extends > > AbstractMessageBundleFactory { > > - > > + static final Logger logger = > > Logger.getLogger(BasicMessageBundleFactory.class.getName()); > > private final HttpFetcher fetcher; > > > > @Override > > - protected FetchedObject<MessageBundle> fetchFromWeb(LocaleSpec locale, > > + protected FetchedObject<MessageBundle> retrieveRawObject(LocaleSpec > > locale, > > boolean ignoreCache) throws GadgetException { > > URI url = locale.getMessages(); > > HttpRequest request = new > > HttpRequest(Uri.fromJavaUri(url)).setIgnoreCache(ignoreCache); > > @@ -60,6 +61,11 @@ > > protected URI getCacheKeyFromQueryObj(LocaleSpec queryObj) { > > return queryObj.getMessages(); > > } > > + > > + @Override > > + protected Logger getLogger() { > > + return logger; > > + } > > > > protected MessageBundle fetchBundle(LocaleSpec locale, boolean > > ignoreCache) > > throws GadgetException { > > > > Modified: > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/CachingWebRetrievalFactory.java > > URL: > > > http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/CachingWebRetrievalFactory.java?rev=689691&r1=689690&r2=689691&view=diff > > > > > ============================================================================== > > --- > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/CachingWebRetrievalFactory.java > > (original) > > +++ > > > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/CachingWebRetrievalFactory.java > > Wed Aug 27 18:16:19 2008 > > @@ -18,20 +18,19 @@ > > */ > > package org.apache.shindig.gadgets; > > > > -import java.net.URI; > > import java.util.logging.Logger; > > > > import org.apache.shindig.common.cache.Cache; > > import org.apache.shindig.common.cache.CacheProvider; > > import org.apache.shindig.gadgets.GadgetException; > > > > -public abstract class CachingWebRetrievalFactory<T, Q> { > > +public abstract class CachingWebRetrievalFactory<T, Q, K> { > > // Subclasses must override these. > > - protected abstract FetchedObject<T> fetchFromWeb(Q queryObj, boolean > > ignoreCache) throws GadgetException; > > - protected abstract URI getCacheKeyFromQueryObj(Q queryObj); > > + protected abstract FetchedObject<T> retrieveRawObject(Q queryObj, > > boolean ignoreCache) throws GadgetException; > > + protected abstract K getCacheKeyFromQueryObj(Q queryObj); > > + protected abstract Logger getLogger(); > > > > - private static final Logger logger = > > Logger.getLogger(CachingWebRetrievalFactory.class.getName()); > > - private final Cache<URI, TimeoutPair<T>> cache; > > + private final Cache<K, TimeoutPair<T>> cache; > > private final long minTtl, maxTtl; > > > > protected CachingWebRetrievalFactory(CacheProvider cacheProvider, > > @@ -48,7 +47,7 @@ > > > > T resultObj = null; > > long expiration = -1; > > - URI cacheKey = getCacheKeyFromQueryObj(queryObj); > > + K cacheKey = getCacheKeyFromQueryObj(queryObj); > > > > synchronized(cache) { > > TimeoutPair<T> cachedEntry = cache.getElement(cacheKey); > > @@ -68,7 +67,7 @@ > > if (resultObj == null) { > > throw e; > > } else { > > - logger.info("Object fetch failed for " + cacheKey + " - > using > > cached "); > > + getLogger().info("Object fetch failed for " + cacheKey + " - > > using cached "); > > // Try again later... > > synchronized (cache) { > > cache.addElement(cacheKey, new TimeoutPair<T>(resultObj, now > + > > minTtl)); > > @@ -81,7 +80,7 @@ > > } > > > > private T fetchObjectAndCache(Q queryObj, boolean ignoreCache) throws > > GadgetException { > > - FetchedObject<T> fetched = fetchFromWeb(queryObj, ignoreCache); > > + FetchedObject<T> fetched = retrieveRawObject(queryObj, ignoreCache); > > > > long now = System.currentTimeMillis(); > > long expiration = fetched.expirationTime; > > > > > > >
