From 116bf57bd673b55f91d8dde7a83fc43e11522ebd Mon Sep 17 00:00:00 2001 From: Aaron Myers Date: Fri, 16 Dec 2011 01:54:44 +0000 Subject: [PATCH] HADOOP-7928. HA: Client failover policy is incorrectly trying to fail over all IOExceptions. Contributed by Aaron T. Myers. git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/branches/HDFS-1623@1215019 13f79535-47bb-0310-9956-ffa450edef68 --- .../hadoop-common/CHANGES.HDFS-1623.txt | 3 + .../apache/hadoop/io/retry/RetryPolicies.java | 2 +- .../hadoop/io/retry/TestFailoverProxy.java | 24 ++++++- .../io/retry/UnreliableImplementation.java | 64 ++++++++----------- 4 files changed, 52 insertions(+), 41 deletions(-) diff --git a/hadoop-common-project/hadoop-common/CHANGES.HDFS-1623.txt b/hadoop-common-project/hadoop-common/CHANGES.HDFS-1623.txt index 56e11457c5..56e1d8f823 100644 --- a/hadoop-common-project/hadoop-common/CHANGES.HDFS-1623.txt +++ b/hadoop-common-project/hadoop-common/CHANGES.HDFS-1623.txt @@ -15,3 +15,6 @@ HADOOP-7922. Improve some logging for client IPC failovers and StandbyExceptions (todd) HADOOP-7921. StandbyException should extend IOException (todd) + +HADOOP-7928. HA: Client failover policy is incorrectly trying to fail over all + IOExceptions (atm) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryPolicies.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryPolicies.java index 5afda59475..a96dc9ee0b 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryPolicies.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryPolicies.java @@ -341,7 +341,7 @@ public RetryAction shouldRetry(Exception e, int retries, failovers == 0 ? 0 : calculateExponentialTime(delayMillis, failovers, maxDelayBase)); } else if (e instanceof SocketException || - e instanceof IOException) { + (e instanceof IOException && !(e instanceof RemoteException))) { if (isMethodIdempotent) { return RetryAction.FAILOVER_AND_RETRY; } else { diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java index b52814cfc1..0a2963f7be 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java @@ -181,7 +181,7 @@ public void testFailoverOnNetworkExceptionIdempotentOperation() assertEquals("impl1", unreliable.succeedsOnceThenFailsReturningString()); try { - assertEquals("impl2", unreliable.succeedsOnceThenFailsReturningString()); + unreliable.succeedsOnceThenFailsReturningString(); fail("should not have succeeded twice"); } catch (IOException e) { // Make sure we *don't* fail over since the first implementation threw an @@ -304,4 +304,26 @@ public void run() { String result = unreliable.failsIfIdentifierDoesntMatch("renamed-impl1"); assertEquals("renamed-impl1", result); } + + /** + * Ensure that normal IO exceptions don't result in a failover. + */ + @Test + public void testExpectedIOException() { + UnreliableInterface unreliable = (UnreliableInterface)RetryProxy + .create(UnreliableInterface.class, + new FlipFlopProxyProvider(UnreliableInterface.class, + new UnreliableImplementation("impl1", TypeOfExceptionToFailWith.REMOTE_EXCEPTION), + new UnreliableImplementation("impl2", TypeOfExceptionToFailWith.UNRELIABLE_EXCEPTION)), + RetryPolicies.failoverOnNetworkException( + RetryPolicies.TRY_ONCE_THEN_FAIL, 10, 1000, 10000)); + + try { + unreliable.failsIfIdentifierDoesntMatch("no-such-identifier"); + fail("Should have thrown *some* exception"); + } catch (Exception e) { + assertTrue("Expected IOE but got " + e.getClass(), + e instanceof IOException); + } + } } \ No newline at end of file diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java index 74a63894d8..185ed2a442 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java @@ -19,6 +19,7 @@ import java.io.IOException; +import org.apache.hadoop.io.retry.UnreliableInterface.UnreliableException; import org.apache.hadoop.ipc.RemoteException; import org.apache.hadoop.ipc.StandbyException; @@ -37,7 +38,8 @@ public class UnreliableImplementation implements UnreliableInterface { public static enum TypeOfExceptionToFailWith { UNRELIABLE_EXCEPTION, STANDBY_EXCEPTION, - IO_EXCEPTION + IO_EXCEPTION, + REMOTE_EXCEPTION } public UnreliableImplementation() { @@ -95,14 +97,7 @@ public String succeedsOnceThenFailsReturningString() if (succeedsOnceThenFailsCount++ < 1) { return identifier; } else { - switch (exceptionToFailWith) { - case STANDBY_EXCEPTION: - throw new StandbyException(identifier); - case UNRELIABLE_EXCEPTION: - throw new UnreliableException(identifier); - case IO_EXCEPTION: - throw new IOException(identifier); - } + throwAppropriateException(exceptionToFailWith, identifier); return null; } } @@ -113,16 +108,8 @@ public String succeedsTenTimesThenFailsReturningString() if (succeedsTenTimesThenFailsCount++ < 10) { return identifier; } else { - switch (exceptionToFailWith) { - case STANDBY_EXCEPTION: - throw new StandbyException(identifier); - case UNRELIABLE_EXCEPTION: - throw new UnreliableException(identifier); - case IO_EXCEPTION: - throw new IOException(identifier); - default: - throw new RuntimeException(identifier); - } + throwAppropriateException(exceptionToFailWith, identifier); + return null; } } @@ -132,16 +119,8 @@ public String succeedsOnceThenFailsReturningStringIdempotent() if (succeedsOnceThenFailsIdempotentCount++ < 1) { return identifier; } else { - switch (exceptionToFailWith) { - case STANDBY_EXCEPTION: - throw new StandbyException(identifier); - case UNRELIABLE_EXCEPTION: - throw new UnreliableException(identifier); - case IO_EXCEPTION: - throw new IOException(identifier); - default: - throw new RuntimeException(identifier); - } + throwAppropriateException(exceptionToFailWith, identifier); + return null; } } @@ -153,17 +132,24 @@ public String failsIfIdentifierDoesntMatch(String identifier) } else { String message = "expected '" + this.identifier + "' but received '" + identifier + "'"; - switch (exceptionToFailWith) { - case STANDBY_EXCEPTION: - throw new StandbyException(message); - case UNRELIABLE_EXCEPTION: - throw new UnreliableException(message); - case IO_EXCEPTION: - throw new IOException(message); - default: - throw new RuntimeException(message); - } + throwAppropriateException(exceptionToFailWith, message); + return null; } } + private static void throwAppropriateException(TypeOfExceptionToFailWith eType, + String message) throws UnreliableException, StandbyException, IOException { + switch (eType) { + case STANDBY_EXCEPTION: + throw new StandbyException(message); + case UNRELIABLE_EXCEPTION: + throw new UnreliableException(message); + case IO_EXCEPTION: + throw new IOException(message); + case REMOTE_EXCEPTION: + throw new RemoteException(IOException.class.getName(), message); + default: + throw new RuntimeException(message); + } + } }