Skip to content

Commit

Permalink
Merge pull request #754 from mattrjacobs/only-fire-fallback-hooks-on-…
Browse files Browse the repository at this point in the history 
…user-supplied-fallbacks

Only fire onFallbackStart/onFallbackError execution hooks if there is a user-supplied fallback
  • Loading branch information
@mattrjacobs
mattrjacobs committed Apr 13, 2015
2 parents a61e079 + e595fd0 commit 3ff3ac7
Show file tree
Hide file tree
Showing 6 changed files with 199 additions and 102 deletions.
43 changes: 41 additions & 2 deletions hystrix-core/src/main/java/com/netflix/hystrix/AbstractCommand.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package com.netflix.hystrix;

import java.lang.ref.Reference;
import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
Expand Down Expand Up @@ -121,6 +122,8 @@ protected static enum TimedOutStatus {
// on the repetitive string processing that will occur on the same classes over and over again
private static ConcurrentHashMap<Class<?>, String> defaultNameCache = new ConcurrentHashMap<Class<?>, String>();

private static ConcurrentHashMap<HystrixCommandKey, Boolean> commandContainsFallback = new ConcurrentHashMap<HystrixCommandKey, Boolean>();

/* package */static String getDefaultNameFromClass(Class<?> cls) {
String fromCache = defaultNameCache.get(cls);
if (fromCache != null) {
Expand Down Expand Up @@ -721,7 +724,9 @@ private Observable<R> getFallbackOrThrowException(final HystrixEventType eventTy

// acquire a permit
if (fallbackSemaphore.tryAcquire()) {
executionHook.onFallbackStart(this);
if (isFallbackUserSupplied(this)) {
executionHook.onFallbackStart(this);
}

try {
fallbackExecutionChain = getFallbackObservable();
Expand Down Expand Up @@ -1055,6 +1060,36 @@ protected TryableSemaphore getExecutionSemaphore() {
}
}

/**
* Each concrete implementation of AbstractCommand should return the name of the fallback method as a String
* This will be used to determine if the fallback "exists" for firing the onFallbackStart/onFallbackError hooks
* @return method name of fallback
*/
protected abstract String getFallbackMethodName();

/**
* For the given command instance, does it define an actual fallback method?
* @param cmd command instance
* @return true iff there is a user-supplied fallback method on the given command instance
*/
/*package-private*/ static boolean isFallbackUserSupplied(final AbstractCommand<?> cmd) {
HystrixCommandKey commandKey = cmd.commandKey;
Boolean containsFromMap = commandContainsFallback.get(commandKey);
if (containsFromMap != null) {
return containsFromMap;
} else {
Boolean toInsertIntoMap;
try {
cmd.getClass().getDeclaredMethod(cmd.getFallbackMethodName());
toInsertIntoMap = true;
} catch (NoSuchMethodException nsme) {
toInsertIntoMap = false;
}
commandContainsFallback.put(commandKey, toInsertIntoMap);
return toInsertIntoMap;
}
}

protected static class ObservableCommand<R> extends Observable<R> {
private final AbstractCommand<R> command;

Expand Down Expand Up @@ -1471,7 +1506,11 @@ private Exception wrapWithOnExecutionErrorHook(Throwable t) {
private Exception wrapWithOnFallbackErrorHook(Throwable t) {
Exception e = getExceptionFromThrowable(t);
try {
return executionHook.onFallbackError(this, e);
if (isFallbackUserSupplied(this)) {
return executionHook.onFallbackError(this, e);
} else {
return e;
}
} catch (Throwable hookEx) {
logger.warn("Error calling ExecutionHook.onFallbackError", hookEx);
return e;
Expand Down
5 changes: 5 additions & 0 deletions hystrix-core/src/main/java/com/netflix/hystrix/HystrixCommand.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,4 +405,9 @@ public Future<R> queue() {
return f;
}

@Override
protected String getFallbackMethodName() {
return "getFallback";
}

}
5 changes: 5 additions & 0 deletions hystrix-core/src/main/java/com/netflix/hystrix/HystrixObservableCommand.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ protected boolean shouldOutputOnNextEvents() {
return true;
}

@Override
protected String getFallbackMethodName() {
return "resumeWithFallback";
}

/**
* Construct a {@link HystrixObservableCommand} with defined {@link Setter} that allows injecting property and strategy overrides and other optional arguments.
* <p>
Expand Down
62 changes: 31 additions & 31 deletions hystrix-core/src/test/java/com/netflix/hystrix/CommonHystrixCommandTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -354,11 +354,11 @@ public void call(C command) {
TestableExecutionHook hook = command.getBuilder().executionHook;
assertTrue(hook.commandEmissionsMatch(0, 1, 0));
assertTrue(hook.executionEventsMatch(0, 1, 0));
assertTrue(hook.fallbackEventsMatch(0, 1, 0));
assertTrue(hook.fallbackEventsMatch(0, 0, 0));
assertEquals(RuntimeException.class, hook.getCommandException().getClass());
assertEquals(RuntimeException.class, hook.getExecutionException().getClass());
assertEquals(UnsupportedOperationException.class, hook.getFallbackException().getClass());
assertEquals("onStart - onThreadStart - !onRunStart - onExecutionStart - onExecutionError - !onRunError - onFallbackStart - onFallbackError - onError - onThreadComplete - ", hook.executionSequence.toString());
assertNull(hook.getFallbackException());
assertEquals("onStart - onThreadStart - !onRunStart - onExecutionStart - onExecutionError - !onRunError - onError - onThreadComplete - ", hook.executionSequence.toString());
}
});
}
Expand Down Expand Up @@ -451,10 +451,10 @@ public void call(C command) {
TestableExecutionHook hook = command.getBuilder().executionHook;
assertTrue(hook.commandEmissionsMatch(0, 1, 0));
assertTrue(hook.executionEventsMatch(0, 0, 0));
assertTrue(hook.fallbackEventsMatch(0, 1, 0));
assertTrue(hook.fallbackEventsMatch(0, 0, 0));
assertEquals(TimeoutException.class, hook.getCommandException().getClass());
assertEquals(UnsupportedOperationException.class, hook.getFallbackException().getClass());
assertEquals("onStart - onThreadStart - !onRunStart - onExecutionStart - onFallbackStart - onFallbackError - onError - ", hook.executionSequence.toString());
assertNull(hook.getFallbackException());
assertEquals("onStart - onThreadStart - !onRunStart - onExecutionStart - onError - ", hook.executionSequence.toString());

try {
Thread.sleep(300);
Expand All @@ -464,8 +464,8 @@ public void call(C command) {

assertTrue(hook.commandEmissionsMatch(0, 1, 0));
assertTrue(hook.executionEventsMatch(1, 0, 1));
assertTrue(hook.fallbackEventsMatch(0, 1, 0));
assertEquals("onStart - onThreadStart - !onRunStart - onExecutionStart - onFallbackStart - onFallbackError - onError - onExecutionEmit - !onRunSuccess - onExecutionSuccess - onThreadComplete - ", hook.executionSequence.toString());
assertTrue(hook.fallbackEventsMatch(0, 0, 0));
assertEquals("onStart - onThreadStart - !onRunStart - onExecutionStart - onError - onExecutionEmit - !onRunSuccess - onExecutionSuccess - onThreadComplete - ", hook.executionSequence.toString());

}
});
Expand Down Expand Up @@ -580,10 +580,10 @@ public void call(C command) {
TestableExecutionHook hook = command.getBuilder().executionHook;
assertTrue(hook.commandEmissionsMatch(0, 1, 0));
assertTrue(hook.executionEventsMatch(0, 0, 0));
assertTrue(hook.fallbackEventsMatch(0, 1, 0));
assertTrue(hook.fallbackEventsMatch(0, 0, 0));
assertEquals(TimeoutException.class, hook.getCommandException().getClass());
assertEquals(UnsupportedOperationException.class, hook.getFallbackException().getClass());
assertEquals("onStart - onThreadStart - !onRunStart - onExecutionStart - onFallbackStart - onFallbackError - onError - ", hook.executionSequence.toString());
assertNull(hook.getFallbackException());
assertEquals("onStart - onThreadStart - !onRunStart - onExecutionStart - onError - ", hook.executionSequence.toString());

try {
Thread.sleep(300);
Expand All @@ -592,9 +592,9 @@ public void call(C command) {
}
assertTrue(hook.commandEmissionsMatch(0, 1, 0));
assertTrue(hook.executionEventsMatch(0, 1, 0));
assertTrue(hook.fallbackEventsMatch(0, 1, 0));
assertTrue(hook.fallbackEventsMatch(0, 0, 0));
assertEquals(RuntimeException.class, hook.getExecutionException().getClass());
assertEquals("onStart - onThreadStart - !onRunStart - onExecutionStart - onFallbackStart - onFallbackError - onError - onExecutionError - !onRunError - onThreadComplete - ", hook.executionSequence.toString());
assertEquals("onStart - onThreadStart - !onRunStart - onExecutionStart - onError - onExecutionError - !onRunError - onThreadComplete - ", hook.executionSequence.toString());

}
});
Expand Down Expand Up @@ -718,10 +718,10 @@ public void call(C command) {
TestableExecutionHook hook = command.getBuilder().executionHook;
assertTrue(hook.commandEmissionsMatch(0, 1, 0));
assertTrue(hook.executionEventsMatch(0, 0, 0));
assertTrue(hook.fallbackEventsMatch(0, 1, 0));
assertTrue(hook.fallbackEventsMatch(0, 0, 0));
assertEquals(RejectedExecutionException.class, hook.getCommandException().getClass());
assertEquals(UnsupportedOperationException.class, hook.getFallbackException().getClass());
assertEquals("onStart - onFallbackStart - onFallbackError - onError - ", hook.executionSequence.toString());
assertNull(hook.getFallbackException());
assertEquals("onStart - onError - ", hook.executionSequence.toString());
}
});
}
Expand Down Expand Up @@ -837,10 +837,10 @@ public void call(C command) {
TestableExecutionHook hook = command.getBuilder().executionHook;
assertTrue(hook.commandEmissionsMatch(0, 1, 0));
assertTrue(hook.executionEventsMatch(0, 0, 0));
assertTrue(hook.fallbackEventsMatch(0, 1, 0));
assertTrue(hook.fallbackEventsMatch(0, 0, 0));
assertEquals(RejectedExecutionException.class, hook.getCommandException().getClass());
assertEquals(UnsupportedOperationException.class, hook.getFallbackException().getClass());
assertEquals("onStart - onFallbackStart - onFallbackError - onError - ", hook.executionSequence.toString());
assertNull(hook.getFallbackException());
assertEquals("onStart - onError - ", hook.executionSequence.toString());
}
});
}
Expand Down Expand Up @@ -940,10 +940,10 @@ public void call(C command) {
TestableExecutionHook hook = command.getBuilder().executionHook;
assertTrue(hook.commandEmissionsMatch(0, 1, 0));
assertTrue(hook.executionEventsMatch(0, 0, 0));
assertTrue(hook.fallbackEventsMatch(0, 1, 0));
assertTrue(hook.fallbackEventsMatch(0, 0, 0));
assertEquals(RuntimeException.class, hook.getCommandException().getClass());
assertEquals(UnsupportedOperationException.class, hook.getFallbackException().getClass());
assertEquals("onStart - onFallbackStart - onFallbackError - onError - ", hook.executionSequence.toString());
assertNull(hook.getFallbackException());
assertEquals("onStart - onError - ", hook.executionSequence.toString());
}
});
}
Expand Down Expand Up @@ -1116,11 +1116,11 @@ public void call(C command) {
TestableExecutionHook hook = command.getBuilder().executionHook;
assertTrue(hook.commandEmissionsMatch(0, 1, 0));
assertTrue(hook.executionEventsMatch(0, 1, 0));
assertTrue(hook.fallbackEventsMatch(0, 1, 0));
assertTrue(hook.fallbackEventsMatch(0, 0, 0));
assertEquals(RuntimeException.class, hook.getCommandException().getClass());
assertEquals(RuntimeException.class, hook.getExecutionException().getClass());
assertEquals(UnsupportedOperationException.class, hook.getFallbackException().getClass());
assertEquals("onStart - !onRunStart - onExecutionStart - onExecutionError - !onRunError - onFallbackStart - onFallbackError - onError - ", hook.executionSequence.toString());
assertNull(hook.getFallbackException());
assertEquals("onStart - !onRunStart - onExecutionStart - onExecutionError - !onRunError - onError - ", hook.executionSequence.toString());
}
});
}
Expand Down Expand Up @@ -1232,10 +1232,10 @@ public void call(C command) {
TestableExecutionHook hook = command.getBuilder().executionHook;
assertTrue(hook.commandEmissionsMatch(0, 1, 0));
assertTrue(hook.executionEventsMatch(0, 0, 0));
assertTrue(hook.fallbackEventsMatch(0, 1, 0));
assertTrue(hook.fallbackEventsMatch(0, 0, 0));
assertEquals(RuntimeException.class, hook.getCommandException().getClass());
assertEquals(UnsupportedOperationException.class, hook.getFallbackException().getClass());
assertEquals("onStart - onFallbackStart - onFallbackError - onError - ", hook.executionSequence.toString());
assertNull(hook.getFallbackException());
assertEquals("onStart - onError - ", hook.executionSequence.toString());
}
});
}
Expand Down Expand Up @@ -1370,10 +1370,10 @@ public void call(C command) {
TestableExecutionHook hook = command.getBuilder().executionHook;
assertTrue(hook.commandEmissionsMatch(0, 1, 0));
assertTrue(hook.executionEventsMatch(0, 0, 0));
assertTrue(hook.fallbackEventsMatch(0, 1, 0));
assertTrue(hook.fallbackEventsMatch(0, 0, 0));
assertEquals(RuntimeException.class, hook.getCommandException().getClass());
assertEquals(UnsupportedOperationException.class, hook.getFallbackException().getClass());
assertEquals("onStart - onFallbackStart - onFallbackError - onError - ", hook.executionSequence.toString());
assertNull(hook.getFallbackException());
assertEquals("onStart - onError - ", hook.executionSequence.toString());
}
});
}
Expand Down

0 comments on commit 3ff3ac7

Please sign in to comment.