On Sat, Oct 18, 2008 at 11:38 AM, <[EMAIL PROTECTED]> wrote:
> Author: etnu
> Date: Sat Oct 18 11:38:25 2008
> New Revision: 705904
>
> URL: http://svn.apache.org/viewvc?rev=705904&view=rev
> Log:
> Added negative caching to BasicGadgetSpecFactory to prevent re-parsing /
> re-fetching of gadgets in a known error state. This saves a substantial
> amount of CPU time over the current implementation, which re-parses on every
> render even when caching is turned on.
>
>
> Modified:
>
>
> incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/BasicGadgetSpecFactory.java
>
>
> incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/BasicGadgetSpecFactoryTest.java
>
> 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=705904&r1=705903&r2=705904&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
> Sat Oct 18 11:38:25 2008
> @@ -40,7 +40,8 @@
> public class BasicGadgetSpecFactory implements GadgetSpecFactory {
> static final String RAW_GADGETSPEC_XML_PARAM_NAME = "rawxml";
> static final Uri RAW_GADGET_URI = Uri.parse("http://localhost/raw.xml";);
> -
> + static final String ERROR_SPEC = "<Module><ModulePrefs
> title='Error'/><Content/></Module>";
> + static final String ERROR_KEY = "parse.exception";
> static final Logger logger =
> Logger.getLogger(BasicGadgetSpecFactory.class.getName());
>
> private final HttpFetcher fetcher;
> @@ -48,7 +49,7 @@
> private final long minTtl;
> private final long maxTtl;
>
> - @Inject
> + @Inject
> public BasicGadgetSpecFactory(HttpFetcher fetcher,
> CacheProvider cacheProvider,
>
> @Named("shindig.gadget-spec.cache.capacity") int capacity,
> @@ -89,15 +90,25 @@
> try {
> return fetchObjectAndCache(uri, ignoreCache);
> } catch (GadgetException e) {
> - // Failed to re-fetch raw object. Use cached object if it exists.
> - if (cached.obj == null) {
> - throw e;
> + // Enforce negative caching.
> + GadgetSpec spec;
> + if (cached.obj != null) {
> + spec = cached.obj;
> } else {
> - logger.info("GadgetSpec fetch failed for " + uri + " - using
> cached.");
> + // We create this dummy spec to avoid the cost of re-parsing
> when a remote site is out.
> + spec = new GadgetSpec(uri, ERROR_SPEC);
> + spec.setAttribute(ERROR_KEY, e);
> + cached.obj = spec;
> }
This fix doesn't structurally seem quite right, and the comment is
incorrect: the re-parse happens when the remote site is in but returns bad
content.
One fundamental issue here is that there's no disambiguation between cause
of GadgetException: parse error vs. fetch exception vs. fetch non-OK status.
In lieu of exception refactoring, it seems to me that it would be best to
put this code in fetchObjectAndCache(...) eg:
GadgetSpec spec = null;
try {
spec = new GadgetSpec(url, response.getResponseAsString());
} catch (GadgetException e) {
// poor form in a method that throws a GadgetException itself maybe, but
boxes in parsing logic
// generate error spec
}
// ...code goes on to (negative) cache the spec as it should.
Reintroduction of negative caching on GadgetExceptions caused in the other
noted ways is of course very important.
>
> + logger.info("GadgetSpec fetch failed for " + uri + " - using
> cached for " + minTtl + " ms");
> + ttlCache.addElementWithTtl(uri, spec, minTtl);
> }
> }
>
> + GadgetException exception = (GadgetException)
> cached.obj.getAttribute(ERROR_KEY);
> + if (exception != null) {
> + throw exception;
> + }
> return cached.obj;
> }
>
>
> Modified:
> incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/BasicGadgetSpecFactoryTest.java
> URL:
> http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/BasicGadgetSpecFactoryTest.java?rev=705904&r1=705903&r2=705904&view=diff
>
> ==============================================================================
> ---
> incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/BasicGadgetSpecFactoryTest.java
> (original)
> +++
> incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/BasicGadgetSpecFactoryTest.java
> Sat Oct 18 11:38:25 2008
> @@ -21,6 +21,7 @@
> import static org.easymock.EasyMock.expect;
> import static org.easymock.classextension.EasyMock.replay;
> import static org.junit.Assert.assertEquals;
> +import static org.junit.Assert.fail;
>
> import org.apache.shindig.common.cache.CacheProvider;
> import org.apache.shindig.common.cache.DefaultCacheProvider;
> @@ -103,7 +104,7 @@
> private final CacheProvider cacheProvider = new DefaultCacheProvider();
>
> private final BasicGadgetSpecFactory specFactory
> - = new BasicGadgetSpecFactory(fetcher, cacheProvider, 5, -1000,
> 1000);
> + = new BasicGadgetSpecFactory(fetcher, cacheProvider, 5, 1000, 1000);
>
> @Test
> public void specFetched() throws Exception {
> @@ -220,6 +221,45 @@
> }
>
> @Test(expected = GadgetException.class)
> + public void badFetchServesCached() throws Exception {
> + HttpRequest firstRequest = new
> HttpRequest(SPEC_URL).setIgnoreCache(false);
> + expect(fetcher.fetch(firstRequest)).andReturn(new
> HttpResponse(LOCAL_SPEC_XML)).once();
> + HttpRequest secondRequest = new
> HttpRequest(SPEC_URL).setIgnoreCache(true);
> +
> expect(fetcher.fetch(secondRequest)).andReturn(HttpResponse.error()).once();
> + replay(fetcher);
> +
> + GadgetSpec original = specFactory.getGadgetSpec(SPEC_URL.toJavaUri(),
> false);
> + GadgetSpec cached = specFactory.getGadgetSpec(SPEC_URL.toJavaUri(),
> true);
> +
> + assertEquals(original.getUrl(), cached.getUrl());
> + assertEquals(original.getChecksum(), cached.getChecksum());
> + }
> +
> + @Test(expected = GadgetException.class)
> + public void malformedGadgetSpecThrows() throws Exception {
> + HttpRequest request = new HttpRequest(SPEC_URL).setIgnoreCache(true);
> + expect(fetcher.fetch(request)).andReturn(new HttpResponse("malformed
> junk"));
> + replay(fetcher);
> +
> + specFactory.getGadgetSpec(SPEC_URL.toJavaUri(), true);
> + }
> +
> + @Test(expected = GadgetException.class)
> + public void malformedGadgetSpecIsCachedAndThrows() throws Exception {
> + HttpRequest request = new HttpRequest(SPEC_URL).setIgnoreCache(false);
> + expect(fetcher.fetch(request)).andReturn(new HttpResponse("malformed
> junk")).once();
> + replay(fetcher);
> +
> + try {
> + specFactory.getGadgetSpec(SPEC_URL.toJavaUri(), false);
> + fail("No exception thrown on bad parse");
> + } catch (GadgetException e) {
> + // Expected.
> + }
> + specFactory.getGadgetSpec(SPEC_URL.toJavaUri(), false);
> + }
> +
> + @Test(expected = GadgetException.class)
> public void throwingFetcherRethrows() throws Exception {
> HttpRequest request = new HttpRequest(SPEC_URL).setIgnoreCache(true);
> expect(fetcher.fetch(request)).andThrow(
>
>
>
