Author: todd Date: Wed Feb 15 18:20:11 2012 New Revision: 1244628 URL: http://svn.apache.org/viewvc?rev=1244628&view=rev Log: HADOOP-8068. void methods can swallow exceptions when going through failover path. Contributed by Todd Lipcon.
Modified: hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/CHANGES.HDFS-1623.txt hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryPolicies.java hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryPolicy.java hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestRetryProxy.java hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java Modified: hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/CHANGES.HDFS-1623.txt URL: http://svn.apache.org/viewvc/hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/CHANGES.HDFS-1623.txt?rev=1244628&r1=1244627&r2=1244628&view=diff ============================================================================== --- hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/CHANGES.HDFS-1623.txt (original) +++ hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/CHANGES.HDFS-1623.txt Wed Feb 15 18:20:11 2012 @@ -47,3 +47,5 @@ HADOOP-8038. Add 'ipc.client.connect.max core-default.xml file. (Uma Maheswara Rao G via atm) HADOOP-8041. Log a warning when a failover is first attempted (todd) + +HADOOP-8068. void methods can swallow exceptions when going through failover path (todd) Modified: hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java URL: http://svn.apache.org/viewvc/hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java?rev=1244628&r1=1244627&r2=1244628&view=diff ============================================================================== --- hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java (original) +++ hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java Wed Feb 15 18:20:11 2012 @@ -90,12 +90,12 @@ class RetryInvocationHandler implements RetryAction action = policy.shouldRetry(e, retries++, invocationFailoverCount, isMethodIdempotent); if (action.action == RetryAction.RetryDecision.FAIL) { - LOG.warn("Exception while invoking " + method.getName() - + " of " + currentProxy.getClass() + ". Not retrying.", e); - if (!method.getReturnType().equals(Void.TYPE)) { - throw e; // non-void methods can't fail without an exception + if (action.reason != null) { + LOG.warn("Exception while invoking " + + currentProxy.getClass() + "." + method.getName() + + ". Not retrying because " + action.reason, e); } - return null; + throw e; } else { // retry or failover // avoid logging the failover if this is the first call on this // proxy object, and we successfully achieve the failover without Modified: hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryPolicies.java URL: http://svn.apache.org/viewvc/hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryPolicies.java?rev=1244628&r1=1244627&r2=1244628&view=diff ============================================================================== --- hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryPolicies.java (original) +++ hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryPolicies.java Wed Feb 15 18:20:11 2012 @@ -56,14 +56,6 @@ public class RetryPolicies { /** * <p> - * Try once, and fail silently for <code>void</code> methods, or by - * re-throwing the exception for non-<code>void</code> methods. - * </p> - */ - public static final RetryPolicy TRY_ONCE_DONT_FAIL = new TryOnceDontFail(); - - /** - * <p> * Keep trying forever. * </p> */ @@ -154,12 +146,6 @@ public class RetryPolicies { static class TryOnceThenFail implements RetryPolicy { public RetryAction shouldRetry(Exception e, int retries, int failovers, boolean isMethodIdempotent) throws Exception { - throw e; - } - } - static class TryOnceDontFail implements RetryPolicy { - public RetryAction shouldRetry(Exception e, int retries, int failovers, - boolean isMethodIdempotent) throws Exception { return RetryAction.FAIL; } } @@ -185,7 +171,7 @@ public class RetryPolicies { public RetryAction shouldRetry(Exception e, int retries, int failovers, boolean isMethodIdempotent) throws Exception { if (retries >= maxRetries) { - throw e; + return RetryAction.FAIL; } return new RetryAction(RetryAction.RetryDecision.RETRY, timeUnit.toMillis(calculateSleepTime(retries))); @@ -325,9 +311,9 @@ public class RetryPolicies { public RetryAction shouldRetry(Exception e, int retries, int failovers, boolean isMethodIdempotent) throws Exception { if (failovers >= maxFailovers) { - LOG.info("Failovers (" + failovers + ") exceeded maximum allowed (" + return new RetryAction(RetryAction.RetryDecision.FAIL, 0, + "failovers (" + failovers + ") exceeded maximum allowed (" + maxFailovers + ")"); - return RetryAction.FAIL; } if (e instanceof ConnectException || @@ -345,7 +331,9 @@ public class RetryPolicies { if (isMethodIdempotent) { return RetryAction.FAILOVER_AND_RETRY; } else { - return RetryAction.FAIL; + return new RetryAction(RetryAction.RetryDecision.FAIL, 0, + "the invoked method is not idempotent, and unable to determine " + + "whether it was invoked"); } } else { return fallbackPolicy.shouldRetry(e, retries, failovers, Modified: hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryPolicy.java URL: http://svn.apache.org/viewvc/hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryPolicy.java?rev=1244628&r1=1244627&r2=1244628&view=diff ============================================================================== --- hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryPolicy.java (original) +++ hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryPolicy.java Wed Feb 15 18:20:11 2012 @@ -44,14 +44,20 @@ public interface RetryPolicy { public final RetryDecision action; public final long delayMillis; + public final String reason; public RetryAction(RetryDecision action) { - this(action, 0); + this(action, 0, null); } public RetryAction(RetryDecision action, long delayTime) { + this(action, delayTime, null); + } + + public RetryAction(RetryDecision action, long delayTime, String reason) { this.action = action; this.delayMillis = delayTime; + this.reason = reason; } public enum RetryDecision { Modified: hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java URL: http://svn.apache.org/viewvc/hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java?rev=1244628&r1=1244627&r2=1244628&view=diff ============================================================================== --- hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java (original) +++ hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java Wed Feb 15 18:20:11 2012 @@ -128,7 +128,7 @@ public class TestFailoverProxy { new FlipFlopProxyProvider(UnreliableInterface.class, new UnreliableImplementation("impl1"), new UnreliableImplementation("impl2")), - RetryPolicies.TRY_ONCE_DONT_FAIL); + RetryPolicies.TRY_ONCE_THEN_FAIL); unreliable.succeedsOnceThenFailsReturningString(); try { @@ -196,6 +196,27 @@ public class TestFailoverProxy { assertEquals("impl2", unreliable.succeedsOnceThenFailsReturningStringIdempotent()); } + /** + * Test that if a non-idempotent void function is called, and there is an exception, + * the exception is properly propagated + */ + @Test + public void testExceptionPropagatedForNonIdempotentVoid() throws Exception { + UnreliableInterface unreliable = (UnreliableInterface)RetryProxy + .create(UnreliableInterface.class, + new FlipFlopProxyProvider(UnreliableInterface.class, + new UnreliableImplementation("impl1", TypeOfExceptionToFailWith.IO_EXCEPTION), + new UnreliableImplementation("impl2", TypeOfExceptionToFailWith.UNRELIABLE_EXCEPTION)), + RetryPolicies.failoverOnNetworkException(1)); + + try { + unreliable.nonIdempotentVoidFailsIfIdentifierDoesntMatch("impl2"); + fail("did not throw an exception"); + } catch (Exception e) { + } + + } + private static class SynchronizedUnreliableImplementation extends UnreliableImplementation { private CountDownLatch methodLatch; Modified: hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestRetryProxy.java URL: http://svn.apache.org/viewvc/hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestRetryProxy.java?rev=1244628&r1=1244627&r2=1244628&view=diff ============================================================================== --- hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestRetryProxy.java (original) +++ hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestRetryProxy.java Wed Feb 15 18:20:11 2012 @@ -19,7 +19,6 @@ package org.apache.hadoop.io.retry; import static org.apache.hadoop.io.retry.RetryPolicies.RETRY_FOREVER; -import static org.apache.hadoop.io.retry.RetryPolicies.TRY_ONCE_DONT_FAIL; import static org.apache.hadoop.io.retry.RetryPolicies.TRY_ONCE_THEN_FAIL; import static org.apache.hadoop.io.retry.RetryPolicies.retryByException; import static org.apache.hadoop.io.retry.RetryPolicies.retryByRemoteException; @@ -59,19 +58,6 @@ public class TestRetryProxy extends Test } } - public void testTryOnceDontFail() throws UnreliableException { - UnreliableInterface unreliable = (UnreliableInterface) - RetryProxy.create(UnreliableInterface.class, unreliableImpl, TRY_ONCE_DONT_FAIL); - unreliable.alwaysSucceeds(); - unreliable.failsOnceThenSucceeds(); - try { - unreliable.failsOnceThenSucceedsWithReturnValue(); - fail("Should fail"); - } catch (UnreliableException e) { - // expected - } - } - public void testRetryForever() throws UnreliableException { UnreliableInterface unreliable = (UnreliableInterface) RetryProxy.create(UnreliableInterface.class, unreliableImpl, RETRY_FOREVER); Modified: hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java URL: http://svn.apache.org/viewvc/hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java?rev=1244628&r1=1244627&r2=1244628&view=diff ============================================================================== --- hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java (original) +++ hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java Wed Feb 15 18:20:11 2012 @@ -136,6 +136,18 @@ public class UnreliableImplementation im return null; } } + + @Override + public void nonIdempotentVoidFailsIfIdentifierDoesntMatch(String identifier) + throws UnreliableException, StandbyException, IOException { + if (this.identifier.equals(identifier)) { + return; + } else { + String message = "expected '" + this.identifier + "' but received '" + + identifier + "'"; + throwAppropriateException(exceptionToFailWith, message); + } + } private static void throwAppropriateException(TypeOfExceptionToFailWith eType, String message) throws UnreliableException, StandbyException, IOException { Modified: hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java URL: http://svn.apache.org/viewvc/hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java?rev=1244628&r1=1244627&r2=1244628&view=diff ============================================================================== --- hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java (original) +++ hadoop/common/branches/HDFS-1623/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java Wed Feb 15 18:20:11 2012 @@ -67,4 +67,7 @@ public interface UnreliableInterface { @Idempotent public String failsIfIdentifierDoesntMatch(String identifier) throws UnreliableException, StandbyException, IOException; + + void nonIdempotentVoidFailsIfIdentifierDoesntMatch(String identifier) + throws UnreliableException, StandbyException, IOException; }