HADOOP-8068. void methods can swallow exceptions when going through failover path. Contributed by Todd Lipcon.
git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/branches/HDFS-1623@1244628 13f79535-47bb-0310-9956-ffa450edef68
This commit is contained in:
parent
011611c765
commit
cdc8146bd4
|
@ -47,3 +47,5 @@ HADOOP-8038. Add 'ipc.client.connect.max.retries.on.timeouts' entry in
|
||||||
core-default.xml file. (Uma Maheswara Rao G via atm)
|
core-default.xml file. (Uma Maheswara Rao G via atm)
|
||||||
|
|
||||||
HADOOP-8041. Log a warning when a failover is first attempted (todd)
|
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)
|
||||||
|
|
|
@ -90,12 +90,12 @@ public Object invoke(Object proxy, Method method, Object[] args)
|
||||||
RetryAction action = policy.shouldRetry(e, retries++, invocationFailoverCount,
|
RetryAction action = policy.shouldRetry(e, retries++, invocationFailoverCount,
|
||||||
isMethodIdempotent);
|
isMethodIdempotent);
|
||||||
if (action.action == RetryAction.RetryDecision.FAIL) {
|
if (action.action == RetryAction.RetryDecision.FAIL) {
|
||||||
LOG.warn("Exception while invoking " + method.getName()
|
if (action.reason != null) {
|
||||||
+ " of " + currentProxy.getClass() + ". Not retrying.", e);
|
LOG.warn("Exception while invoking " +
|
||||||
if (!method.getReturnType().equals(Void.TYPE)) {
|
currentProxy.getClass() + "." + method.getName() +
|
||||||
throw e; // non-void methods can't fail without an exception
|
". Not retrying because " + action.reason, e);
|
||||||
}
|
}
|
||||||
return null;
|
throw e;
|
||||||
} else { // retry or failover
|
} else { // retry or failover
|
||||||
// avoid logging the failover if this is the first call on this
|
// avoid logging the failover if this is the first call on this
|
||||||
// proxy object, and we successfully achieve the failover without
|
// proxy object, and we successfully achieve the failover without
|
||||||
|
|
|
@ -54,14 +54,6 @@ public class RetryPolicies {
|
||||||
*/
|
*/
|
||||||
public static final RetryPolicy TRY_ONCE_THEN_FAIL = new TryOnceThenFail();
|
public static final RetryPolicy TRY_ONCE_THEN_FAIL = new TryOnceThenFail();
|
||||||
|
|
||||||
/**
|
|
||||||
* <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>
|
* <p>
|
||||||
* Keep trying forever.
|
* Keep trying forever.
|
||||||
|
@ -152,12 +144,6 @@ public static final RetryPolicy failoverOnNetworkException(
|
||||||
}
|
}
|
||||||
|
|
||||||
static class TryOnceThenFail implements RetryPolicy {
|
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,
|
public RetryAction shouldRetry(Exception e, int retries, int failovers,
|
||||||
boolean isMethodIdempotent) throws Exception {
|
boolean isMethodIdempotent) throws Exception {
|
||||||
return RetryAction.FAIL;
|
return RetryAction.FAIL;
|
||||||
|
@ -185,7 +171,7 @@ public RetryLimited(int maxRetries, long sleepTime, TimeUnit timeUnit) {
|
||||||
public RetryAction shouldRetry(Exception e, int retries, int failovers,
|
public RetryAction shouldRetry(Exception e, int retries, int failovers,
|
||||||
boolean isMethodIdempotent) throws Exception {
|
boolean isMethodIdempotent) throws Exception {
|
||||||
if (retries >= maxRetries) {
|
if (retries >= maxRetries) {
|
||||||
throw e;
|
return RetryAction.FAIL;
|
||||||
}
|
}
|
||||||
return new RetryAction(RetryAction.RetryDecision.RETRY,
|
return new RetryAction(RetryAction.RetryDecision.RETRY,
|
||||||
timeUnit.toMillis(calculateSleepTime(retries)));
|
timeUnit.toMillis(calculateSleepTime(retries)));
|
||||||
|
@ -325,9 +311,9 @@ public FailoverOnNetworkExceptionRetry(RetryPolicy fallbackPolicy,
|
||||||
public RetryAction shouldRetry(Exception e, int retries,
|
public RetryAction shouldRetry(Exception e, int retries,
|
||||||
int failovers, boolean isMethodIdempotent) throws Exception {
|
int failovers, boolean isMethodIdempotent) throws Exception {
|
||||||
if (failovers >= maxFailovers) {
|
if (failovers >= maxFailovers) {
|
||||||
LOG.info("Failovers (" + failovers + ") exceeded maximum allowed ("
|
return new RetryAction(RetryAction.RetryDecision.FAIL, 0,
|
||||||
|
"failovers (" + failovers + ") exceeded maximum allowed ("
|
||||||
+ maxFailovers + ")");
|
+ maxFailovers + ")");
|
||||||
return RetryAction.FAIL;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
if (e instanceof ConnectException ||
|
if (e instanceof ConnectException ||
|
||||||
|
@ -345,7 +331,9 @@ public RetryAction shouldRetry(Exception e, int retries,
|
||||||
if (isMethodIdempotent) {
|
if (isMethodIdempotent) {
|
||||||
return RetryAction.FAILOVER_AND_RETRY;
|
return RetryAction.FAILOVER_AND_RETRY;
|
||||||
} else {
|
} 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 {
|
} else {
|
||||||
return fallbackPolicy.shouldRetry(e, retries, failovers,
|
return fallbackPolicy.shouldRetry(e, retries, failovers,
|
||||||
|
|
|
@ -44,14 +44,20 @@ public static class RetryAction {
|
||||||
|
|
||||||
public final RetryDecision action;
|
public final RetryDecision action;
|
||||||
public final long delayMillis;
|
public final long delayMillis;
|
||||||
|
public final String reason;
|
||||||
|
|
||||||
public RetryAction(RetryDecision action) {
|
public RetryAction(RetryDecision action) {
|
||||||
this(action, 0);
|
this(action, 0, null);
|
||||||
}
|
}
|
||||||
|
|
||||||
public RetryAction(RetryDecision action, long delayTime) {
|
public RetryAction(RetryDecision action, long delayTime) {
|
||||||
|
this(action, delayTime, null);
|
||||||
|
}
|
||||||
|
|
||||||
|
public RetryAction(RetryDecision action, long delayTime, String reason) {
|
||||||
this.action = action;
|
this.action = action;
|
||||||
this.delayMillis = delayTime;
|
this.delayMillis = delayTime;
|
||||||
|
this.reason = reason;
|
||||||
}
|
}
|
||||||
|
|
||||||
public enum RetryDecision {
|
public enum RetryDecision {
|
||||||
|
|
|
@ -128,7 +128,7 @@ public void testNeverFailOver() throws UnreliableException,
|
||||||
new FlipFlopProxyProvider(UnreliableInterface.class,
|
new FlipFlopProxyProvider(UnreliableInterface.class,
|
||||||
new UnreliableImplementation("impl1"),
|
new UnreliableImplementation("impl1"),
|
||||||
new UnreliableImplementation("impl2")),
|
new UnreliableImplementation("impl2")),
|
||||||
RetryPolicies.TRY_ONCE_DONT_FAIL);
|
RetryPolicies.TRY_ONCE_THEN_FAIL);
|
||||||
|
|
||||||
unreliable.succeedsOnceThenFailsReturningString();
|
unreliable.succeedsOnceThenFailsReturningString();
|
||||||
try {
|
try {
|
||||||
|
@ -196,6 +196,27 @@ public void testFailoverOnNetworkExceptionIdempotentOperation()
|
||||||
assertEquals("impl2", unreliable.succeedsOnceThenFailsReturningStringIdempotent());
|
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 static class SynchronizedUnreliableImplementation extends UnreliableImplementation {
|
||||||
|
|
||||||
private CountDownLatch methodLatch;
|
private CountDownLatch methodLatch;
|
||||||
|
|
|
@ -19,7 +19,6 @@
|
||||||
package org.apache.hadoop.io.retry;
|
package org.apache.hadoop.io.retry;
|
||||||
|
|
||||||
import static org.apache.hadoop.io.retry.RetryPolicies.RETRY_FOREVER;
|
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.TRY_ONCE_THEN_FAIL;
|
||||||
import static org.apache.hadoop.io.retry.RetryPolicies.retryByException;
|
import static org.apache.hadoop.io.retry.RetryPolicies.retryByException;
|
||||||
import static org.apache.hadoop.io.retry.RetryPolicies.retryByRemoteException;
|
import static org.apache.hadoop.io.retry.RetryPolicies.retryByRemoteException;
|
||||||
|
@ -59,19 +58,6 @@ public void testTryOnceThenFail() throws UnreliableException {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
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 {
|
public void testRetryForever() throws UnreliableException {
|
||||||
UnreliableInterface unreliable = (UnreliableInterface)
|
UnreliableInterface unreliable = (UnreliableInterface)
|
||||||
RetryProxy.create(UnreliableInterface.class, unreliableImpl, RETRY_FOREVER);
|
RetryProxy.create(UnreliableInterface.class, unreliableImpl, RETRY_FOREVER);
|
||||||
|
|
|
@ -136,6 +136,18 @@ public String failsIfIdentifierDoesntMatch(String identifier)
|
||||||
return null;
|
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,
|
private static void throwAppropriateException(TypeOfExceptionToFailWith eType,
|
||||||
String message) throws UnreliableException, StandbyException, IOException {
|
String message) throws UnreliableException, StandbyException, IOException {
|
||||||
|
|
|
@ -67,4 +67,7 @@ public String succeedsTenTimesThenFailsReturningString()
|
||||||
@Idempotent
|
@Idempotent
|
||||||
public String failsIfIdentifierDoesntMatch(String identifier)
|
public String failsIfIdentifierDoesntMatch(String identifier)
|
||||||
throws UnreliableException, StandbyException, IOException;
|
throws UnreliableException, StandbyException, IOException;
|
||||||
|
|
||||||
|
void nonIdempotentVoidFailsIfIdentifierDoesntMatch(String identifier)
|
||||||
|
throws UnreliableException, StandbyException, IOException;
|
||||||
}
|
}
|
||||||
|
|
Loading…
Reference in New Issue