From 4b298fe40eedc1327ba11e98817511554cfa5e7a Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Wed, 30 Jan 2019 22:34:26 -0500 Subject: [PATCH] Retrier now uses RetryTemplate --- .../main/java/ca/uhn/fhir/cli/BaseApp.java | 8 +-- hapi-fhir-jpaserver-searchparam/pom.xml | 5 +- .../registry/BaseSearchParamRegistry.java | 28 ++-------- .../fhir/jpa/searchparam/retry/Retrier.java | 49 ++++++++--------- .../jpa/searchparam/retry/RetrierTest.java | 55 +++++++++---------- .../module/cache/SubscriptionLoader.java | 11 +--- .../standalone/SearchParamLoaderTest.java | 15 +---- .../standalone/SubscriptionLoaderTest.java | 13 +---- pom.xml | 6 ++ 9 files changed, 74 insertions(+), 116 deletions(-) mode change 100644 => 100755 hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/BaseApp.java mode change 100644 => 100755 hapi-fhir-jpaserver-searchparam/pom.xml mode change 100644 => 100755 hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/registry/BaseSearchParamRegistry.java mode change 100644 => 100755 hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/retry/Retrier.java mode change 100644 => 100755 hapi-fhir-jpaserver-searchparam/src/test/java/ca/uhn/fhir/jpa/searchparam/retry/RetrierTest.java mode change 100644 => 100755 hapi-fhir-jpaserver-subscription/src/main/java/ca/uhn/fhir/jpa/subscription/module/cache/SubscriptionLoader.java mode change 100644 => 100755 hapi-fhir-jpaserver-subscription/src/test/java/ca/uhn/fhir/jpa/subscription/module/standalone/SearchParamLoaderTest.java mode change 100644 => 100755 hapi-fhir-jpaserver-subscription/src/test/java/ca/uhn/fhir/jpa/subscription/module/standalone/SubscriptionLoaderTest.java mode change 100644 => 100755 pom.xml diff --git a/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/BaseApp.java b/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/BaseApp.java old mode 100644 new mode 100755 index 31aea0978f6..aea44721f1b --- a/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/BaseApp.java +++ b/hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/BaseApp.java @@ -42,9 +42,9 @@ import java.util.List; import static org.fusesource.jansi.Ansi.ansi; public abstract class BaseApp { - public static final String STACKFILTER_PATTERN = "%xEx{full, sun.reflect, org.junit, org.eclipse, java.lang.reflect.Method, org.springframework, org.hibernate, com.sun.proxy, org.attoparser, org.thymeleaf}"; - public static final String STACKFILTER_PATTERN_PROP = "log.stackfilter.pattern"; - public static final String LINESEP = System.getProperty("line.separator"); + private static final String STACKFILTER_PATTERN = "%xEx{full, sun.reflect, org.junit, org.eclipse, java.lang.reflect.Method, org.springframework, org.hibernate, com.sun.proxy, org.attoparser, org.thymeleaf}"; + private static final String STACKFILTER_PATTERN_PROP = "log.stackfilter.pattern"; + static final String LINESEP = System.getProperty("line.separator"); protected static final org.slf4j.Logger ourLog; private static List ourCommands; @@ -145,7 +145,7 @@ public abstract class BaseApp { protected abstract String provideCommandName(); - public List provideCommands() { + List provideCommands() { ArrayList commands = new ArrayList<>(); commands.add(new RunServerCommand()); commands.add(new ExampleDataUploader()); diff --git a/hapi-fhir-jpaserver-searchparam/pom.xml b/hapi-fhir-jpaserver-searchparam/pom.xml old mode 100644 new mode 100755 index c384c0f967e..256da95fc61 --- a/hapi-fhir-jpaserver-searchparam/pom.xml +++ b/hapi-fhir-jpaserver-searchparam/pom.xml @@ -86,7 +86,10 @@ javax.annotation javax.annotation-api - + + org.springframework.retry + spring-retry + diff --git a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/registry/BaseSearchParamRegistry.java b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/registry/BaseSearchParamRegistry.java old mode 100644 new mode 100755 index 8a2d06f59d8..519d0d6cb98 --- a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/registry/BaseSearchParamRegistry.java +++ b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/registry/BaseSearchParamRegistry.java @@ -48,8 +48,6 @@ public abstract class BaseSearchParamRegistry implemen private static final int MAX_MANAGED_PARAM_COUNT = 10000; private static final Logger ourLog = LoggerFactory.getLogger(BaseSearchParamRegistry.class); - @VisibleForTesting - public static final int INITIAL_SECONDS_BETWEEN_RETRIES = 5; private static long REFRESH_INTERVAL = 60 * DateUtils.MILLIS_PER_MINUTE; private static final int MAX_RETRIES = 60; // 5 minutes @@ -60,7 +58,6 @@ public abstract class BaseSearchParamRegistry implemen @Autowired private FhirContext myFhirContext; - private volatile int mySecondsBetweenRetries = INITIAL_SECONDS_BETWEEN_RETRIES; private Map> myBuiltInSearchParams; private volatile Map> myActiveUniqueSearchParams = Collections.emptyMap(); private volatile Map, List>> myActiveParamNamesToUniqueSearchParams = Collections.emptyMap(); @@ -85,7 +82,7 @@ public abstract class BaseSearchParamRegistry implemen return myActiveSearchParams.get(theResourceName); } - void requiresActiveSearchParams() { + private void requiresActiveSearchParams() { if (myActiveSearchParams == null) { refreshCacheWithRetry(); } @@ -120,11 +117,7 @@ public abstract class BaseSearchParamRegistry implemen } private Map getSearchParamMap(Map> searchParams, String theResourceName) { - Map retVal = searchParams.get(theResourceName); - if (retVal == null) { - retVal = new HashMap<>(); - searchParams.put(theResourceName, retVal); - } + Map retVal = searchParams.computeIfAbsent(theResourceName, k -> new HashMap<>()); return retVal; } @@ -139,11 +132,7 @@ public abstract class BaseSearchParamRegistry implemen * Loop through parameters and find JPA params */ for (Map.Entry> nextResourceNameToEntries : theActiveSearchParams.entrySet()) { - List uniqueSearchParams = activeUniqueSearchParams.get(nextResourceNameToEntries.getKey()); - if (uniqueSearchParams == null) { - uniqueSearchParams = new ArrayList<>(); - activeUniqueSearchParams.put(nextResourceNameToEntries.getKey(), uniqueSearchParams); - } + List uniqueSearchParams = activeUniqueSearchParams.computeIfAbsent(nextResourceNameToEntries.getKey(), k -> new ArrayList<>()); Collection nextSearchParamsForResourceName = nextResourceNameToEntries.getValue().values(); for (RuntimeSearchParam nextCandidate : nextSearchParamsForResourceName) { @@ -181,7 +170,7 @@ public abstract class BaseSearchParamRegistry implemen } if (next.getCompositeOf() != null) { - Collections.sort(next.getCompositeOf(), new Comparator() { + next.getCompositeOf().sort(new Comparator() { @Override public int compare(RuntimeSearchParam theO1, RuntimeSearchParam theO2) { return StringUtils.compare(theO1.getName(), theO2.getName()); @@ -192,7 +181,7 @@ public abstract class BaseSearchParamRegistry implemen activeParamNamesToUniqueSearchParams.put(nextBase, new HashMap<>()); } if (!activeParamNamesToUniqueSearchParams.get(nextBase).containsKey(paramNames)) { - activeParamNamesToUniqueSearchParams.get(nextBase).put(paramNames, new ArrayList()); + activeParamNamesToUniqueSearchParams.get(nextBase).put(paramNames, new ArrayList<>()); } activeParamNamesToUniqueSearchParams.get(nextBase).get(paramNames).add(next); } @@ -339,7 +328,7 @@ public abstract class BaseSearchParamRegistry implemen } synchronized int refreshCacheWithRetry() { - Retrier refreshCacheRetrier = new Retrier(() -> mySearchParamProvider.refreshCache(this, REFRESH_INTERVAL), MAX_RETRIES, mySecondsBetweenRetries, "refresh search parameter registry"); + Retrier refreshCacheRetrier = new Retrier(() -> mySearchParamProvider.refreshCache(this, REFRESH_INTERVAL), MAX_RETRIES); return refreshCacheRetrier.runWithRetry(); } @@ -355,11 +344,6 @@ public abstract class BaseSearchParamRegistry implemen } } - @VisibleForTesting - public void setSecondsBetweenRetriesForTesting(int theSecondsBetweenRetries) { - mySecondsBetweenRetries = theSecondsBetweenRetries; - } - @Override public Map> getActiveSearchParams() { requiresActiveSearchParams(); diff --git a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/retry/Retrier.java b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/retry/Retrier.java old mode 100644 new mode 100755 index 4cb987d3127..39b27d090f5 --- a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/retry/Retrier.java +++ b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/retry/Retrier.java @@ -9,9 +9,9 @@ package ca.uhn.fhir.jpa.searchparam.retry; * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -20,9 +20,13 @@ package ca.uhn.fhir.jpa.searchparam.retry; * #L% */ +import org.apache.commons.lang3.Validate; import org.apache.commons.lang3.time.DateUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.springframework.retry.backoff.ExponentialBackOffPolicy; +import org.springframework.retry.policy.SimpleRetryPolicy; +import org.springframework.retry.support.RetryTemplate; import java.util.function.Supplier; @@ -30,34 +34,27 @@ public class Retrier { private static final Logger ourLog = LoggerFactory.getLogger(Retrier.class); private final Supplier mySupplier; - private final int myMaxRetries; - private final int mySecondsBetweenRetries; - private final String myDescription; - public Retrier(Supplier theSupplier, int theMaxRetries, int theSecondsBetweenRetries, String theDescription) { + private final RetryTemplate myRetryTemplate; + + public Retrier(Supplier theSupplier, int theMaxRetries) { + Validate.isTrue(theMaxRetries > 0, "maxRetries must be above zero."); mySupplier = theSupplier; - myMaxRetries = theMaxRetries; - mySecondsBetweenRetries = theSecondsBetweenRetries; - myDescription = theDescription; + + myRetryTemplate = new RetryTemplate(); + + ExponentialBackOffPolicy backOff = new ExponentialBackOffPolicy(); + backOff.setInitialInterval(500); + backOff.setMaxInterval(DateUtils.MILLIS_PER_MINUTE); + backOff.setMultiplier(2); + myRetryTemplate.setBackOffPolicy(backOff); + + SimpleRetryPolicy retryPolicy = new SimpleRetryPolicy(); + retryPolicy.setMaxAttempts(theMaxRetries); + myRetryTemplate.setRetryPolicy(retryPolicy); } public T runWithRetry() { - RuntimeException lastException = new IllegalStateException("maxRetries must be above zero."); - for (int retryCount = 1; retryCount <= myMaxRetries; ++retryCount) { - try { - return mySupplier.get(); - } catch(RuntimeException e) { - ourLog.trace("Failure during retry: {}", e.getMessage(), e); // with stacktrace if it's ever needed - ourLog.info("Failed to {}. Attempt {} / {}: {}", myDescription, retryCount, myMaxRetries, e.getMessage()); - lastException = e; - try { - Thread.sleep(mySecondsBetweenRetries * DateUtils.MILLIS_PER_SECOND); - } catch (InterruptedException ie) { - Thread.currentThread().interrupt(); - throw lastException; - } - } - } - throw lastException; + return myRetryTemplate.execute(retryContext -> mySupplier.get()); } } diff --git a/hapi-fhir-jpaserver-searchparam/src/test/java/ca/uhn/fhir/jpa/searchparam/retry/RetrierTest.java b/hapi-fhir-jpaserver-searchparam/src/test/java/ca/uhn/fhir/jpa/searchparam/retry/RetrierTest.java old mode 100644 new mode 100755 index 60154a2d02a..edc946f09ba --- a/hapi-fhir-jpaserver-searchparam/src/test/java/ca/uhn/fhir/jpa/searchparam/retry/RetrierTest.java +++ b/hapi-fhir-jpaserver-searchparam/src/test/java/ca/uhn/fhir/jpa/searchparam/retry/RetrierTest.java @@ -1,17 +1,24 @@ package ca.uhn.fhir.jpa.searchparam.retry; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Supplier; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; public class RetrierTest { + + @Rule + public ExpectedException myExpectedException = ExpectedException.none(); + @Test public void happyPath() { Supplier supplier = () -> true; - Retrier retrier = new Retrier<>(supplier, 5, 0, "test"); + Retrier retrier = new Retrier<>(supplier, 5); assertTrue(retrier.runWithRetry()); } @@ -22,7 +29,7 @@ public class RetrierTest { if (counter.incrementAndGet() < 3) throw new RetryRuntimeException("test"); return true; }; - Retrier retrier = new Retrier<>(supplier, 5, 0, "test"); + Retrier retrier = new Retrier<>(supplier, 5); assertTrue(retrier.runWithRetry()); assertEquals(3, counter.get()); } @@ -31,16 +38,15 @@ public class RetrierTest { public void failMaxRetries() { AtomicInteger counter = new AtomicInteger(); Supplier supplier = () -> { - if (counter.incrementAndGet() < 10) throw new RetryRuntimeException("test"); + if (counter.incrementAndGet() < 3) throw new RetryRuntimeException("test failure message"); return true; }; - Retrier retrier = new Retrier<>(supplier, 5, 0, "test"); - try { - retrier.runWithRetry(); - fail(); - } catch (RetryRuntimeException e) { - assertEquals(5, counter.get()); - } + Retrier retrier = new Retrier<>(supplier, 1); + + myExpectedException.expect(RetryRuntimeException.class); + myExpectedException.expectMessage("test failure message"); + retrier.runWithRetry(); + assertEquals(5, counter.get()); } @Test @@ -50,14 +56,10 @@ public class RetrierTest { if (counter.incrementAndGet() < 10) throw new RetryRuntimeException("test"); return true; }; - Retrier retrier = new Retrier<>(supplier, 0, 0, "test"); - try { - retrier.runWithRetry(); - fail(); - } catch (IllegalStateException e) { - assertEquals(0, counter.get()); - assertEquals("maxRetries must be above zero." ,e.getMessage()); - } + myExpectedException.expect(IllegalArgumentException.class); + myExpectedException.expectMessage("maxRetries must be above zero."); + Retrier retrier = new Retrier<>(supplier, 0); + assertEquals(0, counter.get()); } @Test @@ -67,18 +69,13 @@ public class RetrierTest { if (counter.incrementAndGet() < 10) throw new RetryRuntimeException("test"); return true; }; - Retrier retrier = new Retrier<>(supplier, -1, 0, "test"); - try { - retrier.runWithRetry(); - fail(); - } catch (IllegalStateException e) { - assertEquals(0, counter.get()); - assertEquals("maxRetries must be above zero." ,e.getMessage()); - } + myExpectedException.expect(IllegalArgumentException.class); + myExpectedException.expectMessage("maxRetries must be above zero."); + + Retrier retrier = new Retrier<>(supplier, -1); + assertEquals(0, counter.get()); } - - class RetryRuntimeException extends RuntimeException { RetryRuntimeException(String message) { super(message); diff --git a/hapi-fhir-jpaserver-subscription/src/main/java/ca/uhn/fhir/jpa/subscription/module/cache/SubscriptionLoader.java b/hapi-fhir-jpaserver-subscription/src/main/java/ca/uhn/fhir/jpa/subscription/module/cache/SubscriptionLoader.java old mode 100644 new mode 100755 index b82b9254137..8fd70e7f358 --- a/hapi-fhir-jpaserver-subscription/src/main/java/ca/uhn/fhir/jpa/subscription/module/cache/SubscriptionLoader.java +++ b/hapi-fhir-jpaserver-subscription/src/main/java/ca/uhn/fhir/jpa/subscription/module/cache/SubscriptionLoader.java @@ -46,8 +46,6 @@ import java.util.concurrent.Semaphore; @Lazy public class SubscriptionLoader { private static final Logger ourLog = LoggerFactory.getLogger(SubscriptionLoader.class); - @VisibleForTesting - public static final int INITIAL_SECONDS_BETWEEN_RETRIES = 5; private static final int MAX_RETRIES = 60; // 60 * 5 seconds = 5 minutes @Autowired @@ -58,8 +56,6 @@ public class SubscriptionLoader { private final Object mySyncSubscriptionsLock = new Object(); private Semaphore mySyncSubscriptionsSemaphore = new Semaphore(1); - private volatile int mySecondsBetweenRetries = INITIAL_SECONDS_BETWEEN_RETRIES; - /** * Read the existing subscriptions from the database */ @@ -82,7 +78,7 @@ public class SubscriptionLoader { } synchronized int doSyncSubscriptionsWithRetry() { - Retrier syncSubscriptionRetrier = new Retrier(() -> doSyncSubscriptions(), MAX_RETRIES, mySecondsBetweenRetries, "sync subscriptions"); + Retrier syncSubscriptionRetrier = new Retrier<>(this::doSyncSubscriptions, MAX_RETRIES); return syncSubscriptionRetrier.runWithRetry(); } @@ -126,10 +122,5 @@ public class SubscriptionLoader { public void setSubscriptionProviderForUnitTest(ISubscriptionProvider theSubscriptionProvider) { mySubscriptionProvidor = theSubscriptionProvider; } - - @VisibleForTesting - public void setSecondsBetweenRetriesForTesting(int theSecondsBetweenRetries) { - mySecondsBetweenRetries = theSecondsBetweenRetries; - } } diff --git a/hapi-fhir-jpaserver-subscription/src/test/java/ca/uhn/fhir/jpa/subscription/module/standalone/SearchParamLoaderTest.java b/hapi-fhir-jpaserver-subscription/src/test/java/ca/uhn/fhir/jpa/subscription/module/standalone/SearchParamLoaderTest.java old mode 100644 new mode 100755 index 753fb291302..62f54735eda --- a/hapi-fhir-jpaserver-subscription/src/test/java/ca/uhn/fhir/jpa/subscription/module/standalone/SearchParamLoaderTest.java +++ b/hapi-fhir-jpaserver-subscription/src/test/java/ca/uhn/fhir/jpa/subscription/module/standalone/SearchParamLoaderTest.java @@ -12,6 +12,7 @@ import org.junit.Test; import org.springframework.beans.factory.annotation.Autowired; import java.util.Arrays; +import java.util.Collections; import static org.junit.Assert.assertEquals; @@ -32,18 +33,8 @@ public class SearchParamLoaderTest extends BaseBlockingQueueSubscribableChannelD myMockFhirClientSearchParamProvider.setFailCount(0); } - @Before - public void zeroRetryDelay() { - mySearchParamRegistry.setSecondsBetweenRetriesForTesting(0); - } - - @After - public void restoreRetryDelay() { - mySearchParamRegistry.setSecondsBetweenRetriesForTesting(mySearchParamRegistry.INITIAL_SECONDS_BETWEEN_RETRIES); - } - @Test - public void testSubscriptionLoaderFhirClientDown() throws Exception { + public void testSubscriptionLoaderFhirClientDown() { String criteria = "BodySite?accessType=Catheter,PD%20Catheter"; SearchParameter sp = new SearchParameter(); @@ -54,7 +45,7 @@ public class SearchParamLoaderTest extends BaseBlockingQueueSubscribableChannelD sp.setXpathUsage(SearchParameter.XPathUsageType.NORMAL); sp.setStatus(Enumerations.PublicationStatus.ACTIVE); - IBundleProvider bundle = new SimpleBundleProvider(Arrays.asList(sp), "uuid"); + IBundleProvider bundle = new SimpleBundleProvider(Collections.singletonList(sp), "uuid"); initSearchParamRegistry(bundle); assertEquals(0, myMockFhirClientSearchParamProvider.getFailCount()); } diff --git a/hapi-fhir-jpaserver-subscription/src/test/java/ca/uhn/fhir/jpa/subscription/module/standalone/SubscriptionLoaderTest.java b/hapi-fhir-jpaserver-subscription/src/test/java/ca/uhn/fhir/jpa/subscription/module/standalone/SubscriptionLoaderTest.java old mode 100644 new mode 100755 index ff675029fe0..cb85aa724f5 --- a/hapi-fhir-jpaserver-subscription/src/test/java/ca/uhn/fhir/jpa/subscription/module/standalone/SubscriptionLoaderTest.java +++ b/hapi-fhir-jpaserver-subscription/src/test/java/ca/uhn/fhir/jpa/subscription/module/standalone/SubscriptionLoaderTest.java @@ -1,6 +1,5 @@ package ca.uhn.fhir.jpa.subscription.module.standalone; -import ca.uhn.fhir.jpa.searchparam.registry.BaseSearchParamRegistry; import ca.uhn.fhir.jpa.subscription.module.cache.SubscriptionLoader; import ca.uhn.fhir.jpa.subscription.module.config.MockFhirClientSubscriptionProvider; import ca.uhn.fhir.rest.api.server.IBundleProvider; @@ -33,18 +32,8 @@ public class SubscriptionLoaderTest extends BaseBlockingQueueSubscribableChannel myMockFhirClientSubscriptionProvider.setFailCount(0); } - @Before - public void zeroRetryDelay() { - mySubscriptionLoader.setSecondsBetweenRetriesForTesting(0); - } - - @After - public void restoreRetryDelay() { - mySubscriptionLoader.setSecondsBetweenRetriesForTesting(BaseSearchParamRegistry.INITIAL_SECONDS_BETWEEN_RETRIES); - } - @Test - public void testSubscriptionLoaderFhirClientDown() throws Exception { + public void testSubscriptionLoaderFhirClientDown() { String payload = "application/fhir+json"; String criteria1 = "Observation?code=SNOMED-CT|" + myCode + "&_format=xml"; diff --git a/pom.xml b/pom.xml old mode 100644 new mode 100755 index e99d52fdaf3..1dea1a9e787 --- a/pom.xml +++ b/pom.xml @@ -554,6 +554,7 @@ 5.1.3.RELEASE 2.1.3.RELEASE 2.1.1.RELEASE + 1.2.2.RELEASE 3.1.4 3.0.11.RELEASE @@ -1265,6 +1266,11 @@ spring-websocket ${spring_version} + + org.springframework.retry + spring-retry + ${spring_retry_version} + org.thymeleaf thymeleaf