Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iss1403: Support @HystrixCommand for rx.Single and rx.Completable sim… #1524

Merged
merged 4 commits into from May 9, 2017

Conversation

dmgcodevil
Copy link
Contributor

Requested by #1403

@mattrjacobs
Copy link
Contributor

Can you add tests showing the case where fallbacks get triggered? Specifically, I think that a mismatch between execution type and fallback type should be disallowed. For example, an execution returning a Completable and a fallback returning an Observable is nonsensical.

…cObservableCommand related to these RX types
@dmgcodevil
Copy link
Contributor Author

dmgcodevil commented Apr 16, 2017

@mattrjacobs thanks for reviewing. I added more test cases for successful scenarios, reviled some issues and resolved them. Regarding a 'failure' scenarios, there is only one special that requires a workaround is Completable, Completable isn't parametrized class but it's still a RX type which is a container in some sense. I added support for RX type because from javanica perspective the following code is correct:

public static class Service {
        @HystrixCommand(fallbackMethod = "fallback")
        public Observable<User> command() { throw new IllegalStateException(); }
        private User fallback() { return null; }
    }

after a little thinking I thought that it makes sense to support this behavior for Completable, so the following code is legit as well:

public static class Service {
        @HystrixCommand(fallbackMethod = "fallback")
        public Completable command() { throw new IllegalStateException(); }
        private User fallback() { return null; }
    }

I will update docs shortly

@dmgcodevil
Copy link
Contributor Author

@mattrjacobs I guess you can merge this one

@dmgcodevil
Copy link
Contributor Author

@mattrjacobs

  • review
  • merge

@mattrjacobs
Copy link
Contributor

Sorry for the delay on this @dmgcodevil . I'm still trying to figure out how much value this adds if hystrix-core doesn't contain the equivalent functionality.

My understanding of this feature is that it will take a user-provided Single/Completable and convert that into a HystrixObservableCommand.

I'm writing a unit test to check that submitting a Completable even works in vanilla Hystrix

@mattrjacobs
Copy link
Contributor

Confirmed this in #1574

@mattrjacobs
Copy link
Contributor

So my thought is that this will help users on hystrix-javanica and I don't see a reason not to merge.

@mattrjacobs mattrjacobs merged commit dd1087a into Netflix:master May 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants