From 61c2b506b6356e2bab180d25e21b5c1ec919f557 Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Fri, 16 Dec 2011 02:18:55 +0000 Subject: [PATCH 1/3] Issue 746: Improve RetryablePredicateTest; fix retry nextMaxInterval --- .../predicates/RetryablePredicate.java | 6 +- .../predicates/RetryablePredicateTest.java | 130 +++++++++++------- 2 files changed, 84 insertions(+), 52 deletions(-) diff --git a/core/src/main/java/org/jclouds/predicates/RetryablePredicate.java b/core/src/main/java/org/jclouds/predicates/RetryablePredicate.java index 9b093bc747..aa2de5a9d8 100644 --- a/core/src/main/java/org/jclouds/predicates/RetryablePredicate.java +++ b/core/src/main/java/org/jclouds/predicates/RetryablePredicate.java @@ -80,6 +80,7 @@ public class RetryablePredicate implements Predicate { } } catch (InterruptedException e) { logger.warn(e, "predicate %s on %s interrupted, returning false", input, predicate); + Thread.currentThread().interrupt(); } catch (RuntimeException e) { if (getFirstThrowableOfType(e, ExecutionException.class) != null) { logger.warn(e, "predicate %s on %s errored [%s], returning false", input, predicate, e.getMessage()); @@ -97,8 +98,9 @@ public class RetryablePredicate implements Predicate { } protected long nextMaxInterval(long attempt, Date end) { - // FIXME i think this should be pow(1.5, attempt) -- or alternatively newInterval = oldInterval*1.5 - long interval = (period * (long) Math.pow(attempt, 1.5)); + // Interval increases exponentially, at a rate of nextInterval *= 1.5 + // Note that attempt starts counting at 1 + long interval = (long) (period * Math.pow(1.5, (attempt-1))); interval = interval > maxPeriod ? maxPeriod : interval; long max = end.getTime() - System.currentTimeMillis(); return (interval > max) ? max : interval; diff --git a/core/src/test/java/org/jclouds/predicates/RetryablePredicateTest.java b/core/src/test/java/org/jclouds/predicates/RetryablePredicateTest.java index 0e7d48dd2a..bd962bfc78 100644 --- a/core/src/test/java/org/jclouds/predicates/RetryablePredicateTest.java +++ b/core/src/test/java/org/jclouds/predicates/RetryablePredicateTest.java @@ -18,26 +18,37 @@ */ package org.jclouds.predicates; -import java.util.Date; +import java.util.ArrayList; +import java.util.List; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import org.testng.Assert; +import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; import com.google.common.base.Predicate; import com.google.common.base.Predicates; +import com.google.common.base.Stopwatch; import com.google.common.base.Supplier; /** * * @author Adrian Cole - * */ -@Test(groups = "unit", sequential = true) +@Test(groups = "unit", singleThreaded = true) public class RetryablePredicateTest { - public static int SLOW_BUILD_SERVER_GRACE = 100; + // Grace must be reasonably big; Thread.sleep can take a bit longer to wake up sometimes... + public static int SLOW_BUILD_SERVER_GRACE = 250; + + private Stopwatch stopwatch; + @BeforeMethod + public void setUp() { + stopwatch = new Stopwatch(); + } + @Test void testFalseOnIllegalStateExeception() { ensureImmediateReturnFor(new IllegalStateException()); @@ -74,7 +85,8 @@ public class RetryablePredicateTest { } }, 3, 1, TimeUnit.SECONDS); - Date startPlusThird = new Date(System.currentTimeMillis() + 1000); + + stopwatch.start(); assert !predicate.apply(new Supplier() { @Override @@ -83,76 +95,94 @@ public class RetryablePredicateTest { } }); - Date now = new Date(); - assert now.compareTo(startPlusThird) < 0 : String.format("%s should be less than %s", now.getTime(), - startPlusThird.getTime()); + long duration = stopwatch.elapsedMillis(); + assertOrdered(duration, SLOW_BUILD_SERVER_GRACE); } @Test void testAlwaysTrue() { + // will call once immediately RetryablePredicate predicate = new RetryablePredicate(Predicates. alwaysTrue(), 3, 1, TimeUnit.SECONDS); - Date startPlusThird = new Date(System.currentTimeMillis() + 1000); + stopwatch.start(); predicate.apply(""); - Date now = new Date(); - assert now.compareTo(startPlusThird) < 0 : String.format("%s should be less than %s", now.getTime(), - startPlusThird.getTime()); + long duration = stopwatch.elapsedMillis(); + assertOrdered(duration, SLOW_BUILD_SERVER_GRACE); } @Test void testAlwaysFalseMillis() { + // maxWait=3; period=1; maxPeriod defaults to 1*10 + // will call at 0, 1, 1+(1*1.5), 3 RetryablePredicate predicate = new RetryablePredicate(Predicates. alwaysFalse(), 3, 1, TimeUnit.SECONDS); - Date startPlus3Seconds = new Date(System.currentTimeMillis() + 3000); - Date startPlus4Seconds = new Date(System.currentTimeMillis() + 4000 + SLOW_BUILD_SERVER_GRACE); + stopwatch.start(); predicate.apply(""); - Date now = new Date(); - assert now.compareTo(startPlus3Seconds) >= 0 : String.format("%s should be less than %s", startPlus3Seconds - .getTime(), now.getTime()); - assert now.compareTo(startPlus4Seconds) <= 0 : String.format("%s should be greater than %s", startPlus4Seconds - .getTime(), now.getTime()); - - } - - private static class ThirdTimeTrue implements Predicate { - - private int count = 0; - - @Override - public boolean apply(String input) { - return count++ == 2; - } - + long duration = stopwatch.elapsedMillis(); + assertOrdered(3000, duration, 3000+SLOW_BUILD_SERVER_GRACE); } @Test void testThirdTimeTrue() { - RetryablePredicate predicate = new RetryablePredicate(new ThirdTimeTrue(), 3, 1, TimeUnit.SECONDS); - - Date startPlus = new Date(System.currentTimeMillis() + 1000); - Date startPlus3 = new Date(System.currentTimeMillis() + 3000 + SLOW_BUILD_SERVER_GRACE); + // maxWait=4; period=1; maxPeriod defaults to 1*10 + // will call at 0, 1, 1+(1*1.5) + RepeatedAttemptsPredicate rawPredicate = new RepeatedAttemptsPredicate(2); + RetryablePredicate predicate = new RetryablePredicate(rawPredicate, 4, 1, TimeUnit.SECONDS); + stopwatch.start(); predicate.apply(""); - Date now = new Date(); - assert now.compareTo(startPlus) >= 0 : String.format("%s should be greater than %s", now.getTime(), startPlus - .getTime()); - assert now.compareTo(startPlus3) <= 0 : String.format("%s should be greater than %s", startPlus3.getTime(), now - .getTime()); + long duration = stopwatch.elapsedMillis(); + + assertOrdered(2500, duration, 2500+SLOW_BUILD_SERVER_GRACE); + assertCallFrequency(rawPredicate.callTimes, 0, 1000, 1000+1500); } @Test void testThirdTimeTrueLimitedMaxInterval() { - RetryablePredicate predicate = new RetryablePredicate(new ThirdTimeTrue(), 3, 1, 1, + // maxWait=3; period=1; maxPeriod=1 + // will call at 0, 1, 1+1 + RepeatedAttemptsPredicate rawPredicate = new RepeatedAttemptsPredicate(2); + RetryablePredicate predicate = new RetryablePredicate(rawPredicate, 3, 1, 1, TimeUnit.SECONDS); - Date startPlus = new Date(System.currentTimeMillis() + 1000); - Date startPlus2 = new Date(System.currentTimeMillis() + 2000 + SLOW_BUILD_SERVER_GRACE); - + stopwatch.start(); predicate.apply(""); - Date now = new Date(); - assert now.compareTo(startPlus) >= 0 : String.format("%s should be greater than %s", now.getTime(), startPlus - .getTime()); - assert now.compareTo(startPlus2) <= 0 : String.format("%s should be greater than %s", startPlus2.getTime(), now - .getTime()); + long duration = stopwatch.elapsedMillis(); + + assertOrdered(2000, duration, 2000+SLOW_BUILD_SERVER_GRACE); + assertCallFrequency(rawPredicate.callTimes, 0, 1000, 2000); } -} \ No newline at end of file + + private static class RepeatedAttemptsPredicate implements Predicate { + final List callTimes = new ArrayList(); + private final int succeedOnAttempt; + private final Stopwatch stopwatch; + private int count = 0; + + RepeatedAttemptsPredicate(int succeedOnAttempt) { + this.succeedOnAttempt = succeedOnAttempt; + this.stopwatch = new Stopwatch(); + stopwatch.start(); + } + @Override + public boolean apply(String input) { + callTimes.add(stopwatch.elapsedMillis()); + return count++ == succeedOnAttempt; + } + } + + private void assertCallFrequency(List actual, Integer... expected) { + Assert.assertEquals(actual.size(), expected.length); + for (int i = 0; i < expected.length; i++) { + long callTime = actual.get(i); + assertOrdered(expected[i], callTime, expected[i]+SLOW_BUILD_SERVER_GRACE); + } + } + + private void assertOrdered(long... vals) { + long prevVal = vals[0]; + for (long val : vals) { + assert val >= prevVal : String.format("%s should be ordered", vals); + } + } +} From aa94c60df91407308246e1e2094cdca85ad12f30 Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Fri, 16 Dec 2011 02:25:01 +0000 Subject: [PATCH 2/3] Issue 746: increasing retry times for init-script --- .../BlockUntilInitScriptStatusIsZeroThenReturnOutput.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/compute/src/main/java/org/jclouds/compute/callables/BlockUntilInitScriptStatusIsZeroThenReturnOutput.java b/compute/src/main/java/org/jclouds/compute/callables/BlockUntilInitScriptStatusIsZeroThenReturnOutput.java index e4f642f129..1d964534e4 100644 --- a/compute/src/main/java/org/jclouds/compute/callables/BlockUntilInitScriptStatusIsZeroThenReturnOutput.java +++ b/compute/src/main/java/org/jclouds/compute/callables/BlockUntilInitScriptStatusIsZeroThenReturnOutput.java @@ -70,6 +70,11 @@ public class BlockUntilInitScriptStatusIsZeroThenReturnOutput extends AbstractFu public BlockUntilInitScriptStatusIsZeroThenReturnOutput( @Named(Constants.PROPERTY_USER_THREADS) ExecutorService userThreads, final ScriptStatusReturnsZero stateRunning, @Assisted final SudoAwareInitManager commandRunner) { + + long retryMaxWait = TimeUnit.DAYS.toMillis(365); // arbitrarily high value, but Long.MAX_VALUE doesn't work! + long retryPeriod = 500; + long retryMaxPeriod = 5000; + this.commandRunner = checkNotNull(commandRunner, "commandRunner"); this.userThreads = checkNotNull(userThreads, "userThreads"); this.notRunningAnymore = new RetryablePredicate(new Predicate() { @@ -78,8 +83,7 @@ public class BlockUntilInitScriptStatusIsZeroThenReturnOutput extends AbstractFu public boolean apply(String arg0) { return commandRunner.runAction(arg0).getOutput().trim().equals(""); } - // arbitrarily high value, but Long.MAX_VALUE doesn't work! - }, TimeUnit.DAYS.toMillis(365)) { + }, retryMaxWait, retryPeriod, retryMaxPeriod, TimeUnit.MILLISECONDS) { /** * make sure we stop the retry loop if someone cancelled the future, this keeps threads * from being consumed on dead tasks From 570fab3dea0a1c1e85020758b80f658774390dba Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Fri, 16 Dec 2011 23:59:33 +0000 Subject: [PATCH 3/3] Issue 746: add properties for initStatusInitialPeriod and initStatusMaxPeriod --- ...nitScriptStatusIsZeroThenReturnOutput.java | 11 ++++++----- .../reference/ComputeServiceConstants.java | 19 +++++++++++++++++-- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/compute/src/main/java/org/jclouds/compute/callables/BlockUntilInitScriptStatusIsZeroThenReturnOutput.java b/compute/src/main/java/org/jclouds/compute/callables/BlockUntilInitScriptStatusIsZeroThenReturnOutput.java index 1d964534e4..ded9d6d971 100644 --- a/compute/src/main/java/org/jclouds/compute/callables/BlockUntilInitScriptStatusIsZeroThenReturnOutput.java +++ b/compute/src/main/java/org/jclouds/compute/callables/BlockUntilInitScriptStatusIsZeroThenReturnOutput.java @@ -28,8 +28,6 @@ import java.util.concurrent.TimeoutException; import javax.annotation.PostConstruct; import javax.annotation.Resource; -import javax.inject.Inject; -import javax.inject.Named; import org.jclouds.Constants; import org.jclouds.compute.domain.ExecResponse; @@ -43,7 +41,9 @@ import org.jclouds.ssh.SshClient; import com.google.common.base.Predicate; import com.google.common.base.Throwables; import com.google.common.util.concurrent.AbstractFuture; +import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; +import com.google.inject.name.Named; /** * A future that works in tandem with a task that was invoked by {@link InitBuilder} @@ -69,11 +69,12 @@ public class BlockUntilInitScriptStatusIsZeroThenReturnOutput extends AbstractFu @Inject public BlockUntilInitScriptStatusIsZeroThenReturnOutput( @Named(Constants.PROPERTY_USER_THREADS) ExecutorService userThreads, + ComputeServiceConstants.InitStatusProperties properties, final ScriptStatusReturnsZero stateRunning, @Assisted final SudoAwareInitManager commandRunner) { long retryMaxWait = TimeUnit.DAYS.toMillis(365); // arbitrarily high value, but Long.MAX_VALUE doesn't work! - long retryPeriod = 500; - long retryMaxPeriod = 5000; + long retryInitialPeriod = properties.initStatusInitialPeriod; + long retryMaxPeriod = properties.initStatusMaxPeriod; this.commandRunner = checkNotNull(commandRunner, "commandRunner"); this.userThreads = checkNotNull(userThreads, "userThreads"); @@ -83,7 +84,7 @@ public class BlockUntilInitScriptStatusIsZeroThenReturnOutput extends AbstractFu public boolean apply(String arg0) { return commandRunner.runAction(arg0).getOutput().trim().equals(""); } - }, retryMaxWait, retryPeriod, retryMaxPeriod, TimeUnit.MILLISECONDS) { + }, retryMaxWait, retryInitialPeriod, retryMaxPeriod, TimeUnit.MILLISECONDS) { /** * make sure we stop the retry loop if someone cancelled the future, this keeps threads * from being consumed on dead tasks diff --git a/compute/src/main/java/org/jclouds/compute/reference/ComputeServiceConstants.java b/compute/src/main/java/org/jclouds/compute/reference/ComputeServiceConstants.java index 745af1cd2f..649a00aca4 100644 --- a/compute/src/main/java/org/jclouds/compute/reference/ComputeServiceConstants.java +++ b/compute/src/main/java/org/jclouds/compute/reference/ComputeServiceConstants.java @@ -36,9 +36,13 @@ public interface ComputeServiceConstants { public static final String PROPERTY_TIMEOUT_NODE_SUSPENDED = "jclouds.compute.timeout.node-suspended"; public static final String PROPERTY_TIMEOUT_SCRIPT_COMPLETE = "jclouds.compute.timeout.script-complete"; public static final String PROPERTY_TIMEOUT_PORT_OPEN = "jclouds.compute.timeout.port-open"; + + public static final String PROPERTY_INIT_STATUS_INITIAL_PERIOD = "jclouds.compute.init-status.initial-period"; + public static final String PROPERTY_INIT_STATUS_MAX_PERIOD = "jclouds.compute.init-status.max-period"; + /** - * comma-separated nodes that we shouldn't attempt to list as they are dead in the provider for - * some reason. + * comma-separated nodes that we shouldn't attempt to list as they are dead + * in the provider for some reason. */ public static final String PROPERTY_BLACKLIST_NODES = "jclouds.compute.blacklist-nodes"; @@ -53,6 +57,17 @@ public interface ComputeServiceConstants { */ public static final String PROPERTY_OS_VERSION_MAP_JSON = "jclouds.compute.os-version-map-json"; + @Singleton + public static class InitStatusProperties { + @Inject(optional = true) + @Named(PROPERTY_INIT_STATUS_INITIAL_PERIOD) + public long initStatusInitialPeriod = 500; + + @Inject(optional = true) + @Named(PROPERTY_INIT_STATUS_MAX_PERIOD) + public long initStatusMaxPeriod = 5000; + } + @Singleton public static class ReferenceData { @Inject(optional = true)