From 988d97b09c06e5b72e6f8780b6dfc556253e602b Mon Sep 17 00:00:00 2001 From: Roman Leventov Date: Fri, 11 Nov 2016 19:25:41 -0600 Subject: [PATCH] Unwrap exceptions from RuntimeException in URIExtractionNamespaceCacheFactory.populateCache() (part of #3667) (#3668) * Unwrap exceptions from RuntimeException in URIExtractionNamespaceCacheFactory.populateCache() * Fix tests --- .../URIExtractionNamespaceCacheFactory.java | 136 +++++++++--------- ...RIExtractionNamespaceCacheFactoryTest.java | 37 +---- 2 files changed, 67 insertions(+), 106 deletions(-) diff --git a/extensions-core/lookups-cached-global/src/main/java/io/druid/server/lookup/namespace/URIExtractionNamespaceCacheFactory.java b/extensions-core/lookups-cached-global/src/main/java/io/druid/server/lookup/namespace/URIExtractionNamespaceCacheFactory.java index b8ddd902578..a97fcff1da1 100644 --- a/extensions-core/lookups-cached-global/src/main/java/io/druid/server/lookup/namespace/URIExtractionNamespaceCacheFactory.java +++ b/extensions-core/lookups-cached-global/src/main/java/io/druid/server/lookup/namespace/URIExtractionNamespaceCacheFactory.java @@ -19,7 +19,6 @@ package io.druid.server.lookup.namespace; -import com.google.common.base.Throwables; import com.google.common.io.ByteSource; import com.google.inject.Inject; import io.druid.data.SearchableVersionedDataFinder; @@ -99,14 +98,12 @@ public class URIExtractionNamespaceCacheFactory implements ExtractionNamespaceCa ); if (uri == null) { - throw new RuntimeException( - new FileNotFoundException( - String.format( - "Could not find match for pattern `%s` in [%s] for %s", - versionRegex, - originalUri, - extractionNamespace - ) + throw new FileNotFoundException( + String.format( + "Could not find match for pattern `%s` in [%s] for %s", + versionRegex, + originalUri, + extractionNamespace ) ); } @@ -116,71 +113,66 @@ public class URIExtractionNamespaceCacheFactory implements ExtractionNamespaceCa final String uriPath = uri.getPath(); - try { - return RetryUtils.retry( - new Callable() + return RetryUtils.retry( + new Callable() + { + @Override + public String call() throws Exception { - @Override - public String call() throws Exception - { - final String version = puller.getVersion(uri); - try { - // Important to call equals() against version because lastVersion could be null - if (version.equals(lastVersion)) { - log.debug( - "URI [%s] for namespace [%s] has the same last modified time [%s] as the last cached. " + - "Skipping ", - uri.toString(), - id, - version - ); - return lastVersion; - } + final String version = puller.getVersion(uri); + try { + // Important to call equals() against version because lastVersion could be null + if (version.equals(lastVersion)) { + log.debug( + "URI [%s] for namespace [%s] has the same last modified time [%s] as the last cached. " + + "Skipping ", + uri.toString(), + id, + version + ); + return lastVersion; } - catch (NumberFormatException ex) { - log.debug(ex, "Failed to get last modified timestamp. Assuming no timestamp"); - } - final ByteSource source; - if (CompressionUtils.isGz(uriPath)) { - // Simple gzip stream - log.debug("Loading gz"); - source = new ByteSource() - { - @Override - public InputStream openStream() throws IOException - { - return CompressionUtils.gzipInputStream(puller.getInputStream(uri)); - } - }; - } else { - source = new ByteSource() - { - @Override - public InputStream openStream() throws IOException - { - return puller.getInputStream(uri); - } - }; - } - final MapPopulator.PopulateResult populateResult = new MapPopulator<>( - extractionNamespace.getNamespaceParseSpec() - .getParser() - ).populate(source, cache); - log.info( - "Finished loading %,d values from %,d lines for namespace [%s]", - populateResult.getEntries(), - populateResult.getLines(), - id - ); - return version; } - }, - puller.shouldRetryPredicate(), - DEFAULT_NUM_RETRIES - ); - } - catch (Exception e) { - throw Throwables.propagate(e); - } + catch (NumberFormatException ex) { + log.debug(ex, "Failed to get last modified timestamp. Assuming no timestamp"); + } + final ByteSource source; + if (CompressionUtils.isGz(uriPath)) { + // Simple gzip stream + log.debug("Loading gz"); + source = new ByteSource() + { + @Override + public InputStream openStream() throws IOException + { + return CompressionUtils.gzipInputStream(puller.getInputStream(uri)); + } + }; + } else { + source = new ByteSource() + { + @Override + public InputStream openStream() throws IOException + { + return puller.getInputStream(uri); + } + }; + } + final MapPopulator.PopulateResult populateResult = new MapPopulator<>( + extractionNamespace.getNamespaceParseSpec() + .getParser() + ).populate(source, cache); + log.info( + "Finished loading %,d values from %,d lines for namespace [%s]", + populateResult.getEntries(), + populateResult.getLines(), + id + ); + return version; + } + }, + puller.shouldRetryPredicate(), + DEFAULT_NUM_RETRIES + ); } } diff --git a/extensions-core/lookups-cached-global/src/test/java/io/druid/server/lookup/namespace/URIExtractionNamespaceCacheFactoryTest.java b/extensions-core/lookups-cached-global/src/test/java/io/druid/server/lookup/namespace/URIExtractionNamespaceCacheFactoryTest.java index 8aff5d46514..1d7542f09f3 100644 --- a/extensions-core/lookups-cached-global/src/test/java/io/druid/server/lookup/namespace/URIExtractionNamespaceCacheFactoryTest.java +++ b/extensions-core/lookups-cached-global/src/test/java/io/druid/server/lookup/namespace/URIExtractionNamespaceCacheFactoryTest.java @@ -40,15 +40,12 @@ import io.druid.server.lookup.namespace.cache.NamespaceExtractionCacheManagersTe import io.druid.server.lookup.namespace.cache.OffHeapNamespaceExtractionCacheManager; import io.druid.server.lookup.namespace.cache.OnHeapNamespaceExtractionCacheManager; import io.druid.server.metrics.NoopServiceEmitter; -import org.hamcrest.BaseMatcher; -import org.hamcrest.Description; import org.joda.time.Period; import org.junit.After; import org.junit.Assert; import org.junit.Before; import org.junit.Rule; import org.junit.Test; -import org.junit.rules.ExpectedException; import org.junit.rules.TemporaryFolder; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -279,8 +276,6 @@ public class URIExtractionNamespaceCacheFactoryTest @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder(); - @Rule - public ExpectedException expectedException = ExpectedException.none(); private final String suffix; private final Function outStreamSupplier; @@ -413,7 +408,7 @@ public class URIExtractionNamespaceCacheFactoryTest Assert.assertEquals(null, map.get("baz")); } - @Test + @Test(expected = FileNotFoundException.class) public void testMissing() throws Exception { URIExtractionNamespace badNamespace = new URIExtractionNamespace( @@ -425,18 +420,10 @@ public class URIExtractionNamespaceCacheFactoryTest ); Assert.assertTrue(new File(namespace.getUri()).delete()); ConcurrentMap map = new ConcurrentHashMap<>(); - try { - factory.populateCache(id, badNamespace, null, map); - } - catch (RuntimeException e) { - Assert.assertNotNull(e.getCause()); - Assert.assertEquals(FileNotFoundException.class, e.getCause().getClass()); - return; - } - Assert.fail("Did not have exception"); + factory.populateCache(id, badNamespace, null, map); } - @Test + @Test(expected = FileNotFoundException.class) public void testMissingRegex() throws Exception { String badId = "bad"; @@ -450,24 +437,6 @@ public class URIExtractionNamespaceCacheFactoryTest ); Assert.assertTrue(new File(namespace.getUri()).delete()); ConcurrentMap map = new ConcurrentHashMap<>(); - expectedException.expect(new BaseMatcher() - { - @Override - public void describeTo(Description description) - { - - } - - @Override - public boolean matches(Object o) - { - if (!(o instanceof Throwable)) { - return false; - } - final Throwable t = (Throwable) o; - return t.getCause() != null && t.getCause() instanceof FileNotFoundException; - } - }); factory.populateCache(badId, badNamespace, null, map); }