Doesn't that depend on the key you use for rewrite caching? If you use, as I was thinking, URI+view, then you need to take expiration into account because the base gadgetspec+view contents expire per spec cache control. I suppose we could use a hash of the input though, instead. Meanwhile, btw, I'm closing in on a TTL cache class that we could maybe use as a compromise to get rid of CachingWebRetrievalFactory.
--John On Thu, Aug 28, 2008 at 1:51 PM, Kevin Brown <[EMAIL PROTECTED]> wrote: > On Thu, Aug 28, 2008 at 12:12 PM, John Hjelmstad <[EMAIL PROTECTED]> wrote: > > > 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. > > > But it's *not* the same caching logic. The caching logic used for manifest > files is intentionally different from that used for network fetches (or > rewriting even), because it has to be aware of the retry logic, whereas > rewriting doesn't have that issue because it's not doing a network fetch. > > For a manifest file, the logic is: > > Look up in cache > ---- Exists, but is expired -> Fetch from web > -------- Success -> Store to cache and return. > -------- Failure -> Use old entry and update TTL. > ---- Exists, not expired -> Return > ---- Does not exist -> Fetch from web > -------- Success -> Cache and return > -------- Failure -> Throw error > > For rewriting though, the logic is simply: > > Look up in cache > ---- Exists -> Return > ---- Does not exist -> Rewrite, store to cache, and return > > Which is similar to the logic used by http caching: > Look up in cache > ---- Exists -> Return > ---- Does not exist -> Refetch > -------- Success -> Store, Return > -------- Failure -> Store (negative caching TTL), Return > > These are completely different pieces of code, and they need different > behavior. > > Manifest files must be cached in local memory for performance reasons, but > network fetches and rewriting will almost always be on a shared cache due > to > size constraints. > > > > 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; > > > > > > > > > > > > > > > > > >
