You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by et...@apache.org on 2008/10/18 20:38:25 UTC
svn commit: r705904 - in /incubator/shindig/trunk/java/gadgets/src:
main/java/org/apache/shindig/gadgets/BasicGadgetSpecFactory.java
test/java/org/apache/shindig/gadgets/BasicGadgetSpecFactoryTest.java
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;
}
+ 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(
Re: svn commit: r705904 - in /incubator/shindig/trunk/java/gadgets/src: main/java/org/apache/shindig/gadgets/BasicGadgetSpecFactory.java test/java/org/apache/shindig/gadgets/BasicGadgetSpecFactoryTest.java
Posted by Kevin Brown <et...@google.com>.
On Mon, Oct 20, 2008 at 12:33 PM, John Hjelmstad <fa...@google.com> wrote:
> 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:
That's actually a good thing. We don't want to waste time going back to the
network when we know the last check was in error (for any reason). The only
time that this becomes an issue is if your min ttl is excessively high.
>
> 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.
>
Re: svn commit: r705904 - in /incubator/shindig/trunk/java/gadgets/src: main/java/org/apache/shindig/gadgets/BasicGadgetSpecFactory.java test/java/org/apache/shindig/gadgets/BasicGadgetSpecFactoryTest.java
Posted by John Hjelmstad <fa...@google.com>.
On Sat, Oct 18, 2008 at 11:38 AM, <et...@apache.org> 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(
>
>
>