On Thu, Aug 28, 2008 at 3:08 PM, John Hjelmstad <[EMAIL PROTECTED]> wrote:
> I think I need to circumscribe my comments a little more here. What I'm
> implementing - purely as a steppingstone to getting in GadgetSpec
> immutability/Gadget mutability - is only a cache for rewritten gadget
> contents, in order to maintain _some_ such cache (since making GadgetSpec
> immutable means removing rewriting from BasicGadgetSpecFactory, and thus
> removing its caching behavior from rewritten gadget contents). In that
> case,
> URI+view maintains equivalent caching behavior to before.
> I'd love to fix AbstractHttpCache/the general notion of rewritten-content
> caching/ContentRewriter interface all in one fell swoop, but the code just
> for GadgetSpec immutability pushes the size I'd prefer for a given CL. So
> I'm phasing in functionality this way, before consolidating rewriter
> caching
> in ways such as you describe.
So, you're adding about a hundred lines of (extremely confusing, hard to
follow) code to avoid removing roughly the same amount of code?
To fix the rewriting, delete all rewriting related code from
AbstractHttpCache and from BasicGadgetSpecFactory.
Next, change the registry. The registry should be invoking the rewriters and
handling caching, as relying on all the calling code to do it is bizarre and
violates every principal of OO design there is. You can make it mimic the
ContentRewriter interface for now to simplify this.
Then, add calls to rewrite to GadgetRenderingTask, ProxyHandler, and
MakeRequestHandler. These should just be a few lines such as:
(in GadgetRenderingTask)
String content = view.getContent();
String rewritten = rewriterRegistry.rewriteGadgetView(gadget, content,
"text/html");
if (rewritten != null) {
content = rewritten;
}
This change is tiny compared to what you're trying to do (and what you
already committed), and it meets all of the goals for fixing the rewriting
interface.
I'd even go so far here as to say that it's *OK* to temporarily remove all
caching of rewriting and add it back in in a separate patch. It isn't like
we're going to be doing a release of what's in SVN right now anyway.
>
>
> John
>
> On Thu, Aug 28, 2008 at 3:02 PM, Kevin Brown <[EMAIL PROTECTED]> wrote:
>
> > On Thu, Aug 28, 2008 at 2:32 PM, John Hjelmstad <[EMAIL PROTECTED]>
> wrote:
> >
> > > 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.
> >
> >
> > Again, only if you're still assuming that most / all rewriting is for
> > gadgets, which it isn't. On orkut for instance, it's less than 10% of
> > rewritten data.
> >
> > Even if you have a TTL on rewritten content (which I think is wholly
> > unnecessary, since you can always use an arbitrary cache key), the
> behavior
> > is still fundamentally different, and should be treated as such. This
> isn't
> > a good place try to reuse code, as the non-reused code is simpler and
> > clearer.
> >
> > gadget uri + view is absolutely not an appropriate key for rewriting in
> any
> > case. What if I'm rewriting a transitively proxied link from within a
> > gadget? I'd wind up getting the exact same content no matter what I
> clicked
> > on.
> >
> >
> > >
> > >
> > > --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;
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
