Fix Javanica unit tests for fallback commands #663
Comments
|
I'll try to take a look at it and see what I can make out of it. |
|
Thanks @sawano! On Mon, Feb 9, 2015 at 11:36 PM, Daniel Sawano notifications@github.com
|
|
Hi @mattrjacobs I've looked into it, the first that I found is that the broken tests dedicated to check nested fallbacks calls. Test testGetUserSyncWithFallbackCommand covers the following scenario:
ok, I decided that it would be better to create a test in order to check this approach (command-firstFallback-secondFallback-plainFallback) works fine without javanica. Thus I created test: public class MultiFallbackTest {
public static final String MAIN_COMMAND = MainCommand.class.getSimpleName();
public static final String FIRST_FALLBACK = FirstFallback.class.getSimpleName();
public static final String SECOND_FALLBACK = SecondFallback.class.getSimpleName();
public static class MainCommand extends HystrixCommand<String> {
protected MainCommand() {
super(buildSetter(MAIN_COMMAND));
}
@Override
protected String run() throws Exception {
throw new RuntimeException();
}
@Override protected String getFallback() {
try {
return new FirstFallback().execute();
} catch (Exception e) {
throw Throwables.propagate(e);
}
}
}
public static class FirstFallback extends HystrixCommand<String> {
protected FirstFallback() {
super(buildSetter(FIRST_FALLBACK));
}
@Override
protected String run() throws Exception {
throw new RuntimeException();
}
@Override protected String getFallback() {
try {
return new SecondFallback().execute();
} catch (Exception e) {
throw Throwables.propagate(e);
}
}
}
public static class SecondFallback extends HystrixCommand<String> {
protected SecondFallback() {
super(buildSetter(SECOND_FALLBACK));
}
@Override protected String run() throws Exception {
throw new RuntimeException();
}
@Override protected String getFallback() {
return "result";
}
}
private static HystrixCommand.Setter buildSetter(String commandKey) {
return HystrixCommand.Setter
.withGroupKey(HystrixCommandGroupKey.Factory.asKey(MultiFallbackTest.class.getSimpleName()))
.andCommandKey(HystrixCommandKey.Factory.asKey(commandKey));
}
@Test
public void test() throws Exception {
HystrixRequestContext context = HystrixRequestContext.initializeContext();
try {
assertEquals("result", new MainCommand().execute());
assertEquals(3, HystrixRequestLog.getCurrentRequest().getAllExecutedCommands().size());
HystrixInvokableInfo<?> mainCommand = getHystrixCommandByKey(MAIN_COMMAND);
com.netflix.hystrix.HystrixInvokableInfo firstFallbackCommand = getHystrixCommandByKey(FIRST_FALLBACK);
com.netflix.hystrix.HystrixInvokableInfo secondFallbackCommand = getHystrixCommandByKey(SECOND_FALLBACK);
// confirm that command has failed
assertTrue(mainCommand.getExecutionEvents().contains(HystrixEventType.FAILURE));
// confirm that first fallback has failed
assertTrue(firstFallbackCommand.getExecutionEvents().contains(HystrixEventType.FAILURE));
// and that second fallback was successful
assertTrue(secondFallbackCommand.getExecutionEvents().contains(HystrixEventType.FALLBACK_SUCCESS));
} finally {
context.shutdown();
}
}
}This test fails with the following error (including stacktrace): It looks like something changed in core that affects this behavior and root cause of the issue isn't because of javanica. @mattrjacobs could you say what major changes were made in core before the tests became broken, any information will be really helpful for me. |
|
Thanks for looking into it, @dmgcodevil. The change that triggered the unit test failure was a bugfix. In 1.3.x, setting the interruptOnTimeout property to true for a thread-isolated HystrixCommand caused an In the migration to 1.4, this logic got dropped for the nonblocking timeout case, and the thread interruption was never occurring. The #647 PR added this capability back. Since the Javanica unit tests were the only ones to fail, I assumed the issue was with Javanica. Given your reproduction above, it looks like the issue could be in Hystrix-core. I'll add in your unit tests and see what I find. I really appreciate the help! |
|
Javanica is built over the core, it doesn't introduce some specific logic that decoupled from it, thus if the test case that I mentioned above will pass then the broken javanica test will returned back to normal state. |
|
Do you have any tests to check such behaviour, that is: a command with a fallback which is also a comnand and so on and so forth |
|
I'm adding a few right now. They are failing, as expected given your example above. |
|
Here are a few reproductions: #670. I'm working on a fix. |
|
I see you completely changed core code to use rx, or it was made from scratch (originally) ? I'm asking because I would like to have a facility to read core source code in order to find root cause of an issues if it take place, I think It can be useful if you create separate article where you will describe how the core organized, add some visual diagrams of core architecture, because when I'm looking into code I see callback hell and it's really not easy to find something because of lack of information about infrastructure. WDYT ? |
|
@dmgcodevil Yes, the major change from 1.3.x -> 1.4 was to base the command execution flow on RxJava. This allows for true non-blocking usage, as in |
|
@mattrjacobs it sounds good, thanks. Also, please keep me posted about progress with faiing tests. |
|
Root cause here was that thread interruption was happening on every |
|
Great! Thanks. |
These starting failing in #647. Commented them out to make forward progress. I won't be able to release until these get resolved. /cc @dmgcodevil @sawano for any insights
The text was updated successfully, but these errors were encountered: