You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by jo...@apache.org on 2008/08/28 03:16:19 UTC
svn commit: r689691 - in
/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets:
AbstractMessageBundleFactory.java BasicGadgetSpecFactory.java
BasicMessageBundleFactory.java CachingWebRetrievalFactory.java
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;
Re: Relative Path for thumbnail ?
Posted by Kevin Brown <et...@google.com>.
That's the expected behavior based on the current spec (substitutions are
just strings).
There has been some discussion about making all urls relative (see
opensocial discussion archives), but that has not been adopted as a formal
part of the specification thus far.
On Thu, Aug 28, 2008 at 4:17 PM, Viji Subramanian <
Viji.Subramanian@corp.aol.com> wrote:
> Kevin,
>
> <ModulePrefs title="__MSG_gadgetTitle__" title_url="http://xyz.com"
> description="__MSG_GadgetDesc__" author="test" author_email="test@xyz.com"
> screenshot="" thumbnail="__MSG_thumbnail__">
>
> I noticed that when the message bundle substitution occurs and if the
> thumbnail value in the messagebundle xml file has a relative path, then
> shindig just substitutes__MSG_thumbnail__ with the relative path, it doesn't
> seem to generate the absolute path based on the gadget url.
>
> Is this something that is being taken care in the recent checkins or in the
> todo list ?
>
> It is not a critical thing - as we can always figure out the absolute path
> for thumbnail url, but i think the substitution should handle it.
>
> Thanks
>
Relative Path for thumbnail ?
Posted by Viji Subramanian <Vi...@corp.aol.com>.
Kevin,
<ModulePrefs title="__MSG_gadgetTitle__" title_url="http://xyz.com"
description="__MSG_GadgetDesc__" author="test"
author_email="test@xyz.com" screenshot="" thumbnail="__MSG_thumbnail__">
I noticed that when the message bundle substitution occurs and if the
thumbnail value in the messagebundle xml file has a relative path, then
shindig just substitutes__MSG_thumbnail__ with the relative path, it
doesn't seem to generate the absolute path based on the gadget url.
Is this something that is being taken care in the recent checkins or in
the todo list ?
It is not a critical thing - as we can always figure out the absolute
path for thumbnail url, but i think the substitution should handle it.
Thanks
Re: svn commit: r689691 - in /incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets: AbstractMessageBundleFactory.java BasicGadgetSpecFactory.java BasicMessageBundleFactory.java CachingWebRetrievalFactory.java
Posted by Kevin Brown <et...@google.com>.
On Thu, Aug 28, 2008 at 3:08 PM, John Hjelmstad <fa...@google.com> 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 <et...@google.com> wrote:
>
> > On Thu, Aug 28, 2008 at 2:32 PM, John Hjelmstad <fa...@google.com>
> 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 <et...@google.com> wrote:
> > >
> > > > On Thu, Aug 28, 2008 at 12:12 PM, John Hjelmstad <fa...@google.com>
> > > 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 <et...@google.com>
> > 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, <jo...@apache.org> 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;
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: svn commit: r689691 - in /incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets: AbstractMessageBundleFactory.java BasicGadgetSpecFactory.java BasicMessageBundleFactory.java CachingWebRetrievalFactory.java
Posted by John Hjelmstad <fa...@google.com>.
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.
John
On Thu, Aug 28, 2008 at 3:02 PM, Kevin Brown <et...@google.com> wrote:
> On Thu, Aug 28, 2008 at 2:32 PM, John Hjelmstad <fa...@google.com> 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 <et...@google.com> wrote:
> >
> > > On Thu, Aug 28, 2008 at 12:12 PM, John Hjelmstad <fa...@google.com>
> > 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 <et...@google.com>
> 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, <jo...@apache.org> 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;
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: svn commit: r689691 - in /incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets: AbstractMessageBundleFactory.java BasicGadgetSpecFactory.java BasicMessageBundleFactory.java CachingWebRetrievalFactory.java
Posted by Kevin Brown <et...@google.com>.
On Thu, Aug 28, 2008 at 2:32 PM, John Hjelmstad <fa...@google.com> 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 <et...@google.com> wrote:
>
> > On Thu, Aug 28, 2008 at 12:12 PM, John Hjelmstad <fa...@google.com>
> 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 <et...@google.com> 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, <jo...@apache.org> 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;
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
>
Re: svn commit: r689691 - in /incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets: AbstractMessageBundleFactory.java BasicGadgetSpecFactory.java BasicMessageBundleFactory.java CachingWebRetrievalFactory.java
Posted by John Hjelmstad <fa...@google.com>.
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 <et...@google.com> wrote:
> On Thu, Aug 28, 2008 at 12:12 PM, John Hjelmstad <fa...@google.com> 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 <et...@google.com> 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, <jo...@apache.org> 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;
> > > >
> > > >
> > > >
> > >
> >
>
Re: svn commit: r689691 - in /incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets: AbstractMessageBundleFactory.java BasicGadgetSpecFactory.java BasicMessageBundleFactory.java CachingWebRetrievalFactory.java
Posted by Kevin Brown <et...@google.com>.
On Thu, Aug 28, 2008 at 12:12 PM, John Hjelmstad <fa...@google.com> 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 <et...@google.com> 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, <jo...@apache.org> 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;
> > >
> > >
> > >
> >
>
Re: svn commit: r689691 - in /incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets: AbstractMessageBundleFactory.java BasicGadgetSpecFactory.java BasicMessageBundleFactory.java CachingWebRetrievalFactory.java
Posted by John Hjelmstad <fa...@google.com>.
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 <et...@google.com> 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, <jo...@apache.org> 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;
> >
> >
> >
>
Re: svn commit: r689691 - in /incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets: AbstractMessageBundleFactory.java BasicGadgetSpecFactory.java BasicMessageBundleFactory.java CachingWebRetrievalFactory.java
Posted by Kevin Brown <et...@google.com>.
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, <jo...@apache.org> 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;
>
>
>