Hystrix 1.4 API Design Review

[benjchristensen]

The design of Hystrix 1.4 needs to evolve based on experience using the release candidates. This is the primary reason I haven't shipped a final 1.4.0 ... not having had the confidence in the APIs and reserving the right to change them. Discussions in #317, #315, #223 and internally at Netflix have shaped some changes currently committed but not yet released.

HystrixAsyncCommand

This is new. It is to provide non-blocking functionality like HystrixObservableCommand but only support scalar responses. This is to address the nuances of fallback and timeout behavior with streaming results (see #317 and #315) and the issue with the generics on HystrixExecutable not working with streaming.

Here are particular pieces of code worth reviewing:

run() ->

Hystrix/hystrix-core/src/main/java/com/netflix/hystrix/HystrixAsyncCommand.java

Line 261 in 62ac93f

protected abstract HystrixFuture<R> run();

HystrixFuture -

Hystrix/hystrix-core/src/main/java/com/netflix/hystrix/HystrixAsyncCommand.java

Line 197 in 62ac93f

public static final class HystrixFuture<R> {

Promise -

Hystrix/hystrix-core/src/main/java/com/netflix/hystrix/HystrixAsyncCommand.java

Line 232 in 62ac93f

public static final class Promise<R> {

I’m not convinced I’ve got the HystrixFuture/Promise API or names correct. Functionally it seems correct though.

HystrixObservableCommand

This is functionally the same as HystrixObservableCommand during all of the 1.4.0 release candidates, except it removes the execute and queue methods and doesn't implement HystrixExecutable because of the generics issues (#315).

It also renames the run and getFallback methods to be more clear:

construct() -

Hystrix/hystrix-core/src/main/java/com/netflix/hystrix/HystrixObservableCommand.java

Line 191 in 62ac93f

protected abstract Observable<R> construct();

onFailureResumeWithFallback() -

Hystrix/hystrix-core/src/main/java/com/netflix/hystrix/HystrixObservableCommand.java

Line 208 in 62ac93f

protected Observable<R> onFailureResumeWithFallback() {

I don’t like the name of the ‘onFailureResumeWithFallback’ but getFallback() doesn’t work. constructFallback() would work, but I was trying to communicate the fact that it is “resuming” after failure.
What do you think we should do here?

Here is a unit test showing the partial success with fallback use case:

Hystrix/hystrix-core/src/test/java/com/netflix/hystrix/HystrixObservableCommandTest.java

Line 6287 in 62ac93f

public void testExecutionPartialSuccessWithMoreIntelligentFallback() {

Interfaces

I have had to massage the interfaces because HystrixObservableCommand can’t implement HystrixExecutable. The plugins need something that all 3 implement. They can do HystrixObservable but that seems too focused so I am using a marker interface (HystrixInvokable) despite not really liking marker interfaces.

It means the old stuff is deprecated because I can’t break the APIs. Not ideal but I think it’s working.

https://github.com/Netflix/Hystrix/blob/62ac93f94c7248c2a5bac596344bac0f332fdf7f/hystrix-core/src/main/java/com/netflix/hystrix/HystrixExecutable.java (This can’t be implemented by HystrixObservableCommand)
https://github.com/Netflix/Hystrix/blob/62ac93f94c7248c2a5bac596344bac0f332fdf7f/hystrix-core/src/main/java/com/netflix/hystrix/HystrixExecutableInfo.java
https://github.com/Netflix/Hystrix/blob/62ac93f94c7248c2a5bac596344bac0f332fdf7f/hystrix-core/src/main/java/com/netflix/hystrix/HystrixInvokable.java
https://github.com/Netflix/Hystrix/blob/62ac93f94c7248c2a5bac596344bac0f332fdf7f/hystrix-core/src/main/java/com/netflix/hystrix/HystrixObservable.java

Collapsers

https://github.com/Netflix/Hystrix/blob/62ac93f94c7248c2a5bac596344bac0f332fdf7f/hystrix-core/src/main/java/com/netflix/hystrix/HystrixObservableCollapser.java
https://github.com/Netflix/Hystrix/blob/62ac93f94c7248c2a5bac596344bac0f332fdf7f/hystrix-core/src/main/java/com/netflix/hystrix/HystrixCollapser.java

I still need to add HystrixAsyncCollapser.
Note that HystrixObservableCollapser is different than HystrixCollapser in how the functions are applied. AsyncCollapser will be similar but need to address a batch response type as a scalar value rather than a stream.

---

I would appreciate a review of the public APIs before locking this down and releasing 1.4.0 Final.

[benjchristensen]

/cc @KoltonAndrus @mattrjacobs

[benjchristensen]

Note that the reason for HystrixAsyncCommand creating its own Future/Promise is because we don't want to add a dependency to another library to get just these interfaces (such as ListenableFuture from Guava) and we can't leverage Java8 CompletableFuture as this code compiles to Java 6. The intent with these types is to have the bare minimum interface so they can bridge to any other Future/Task/Observable/etc.

[benjchristensen]

Here is async command usage:

public class ExampleAsyncCommand extends HystrixAsyncCommand<String> {

    protected ExampleAsyncCommand() {
        super(HystrixCommandGroupKey.Factory.asKey("ExampleAsync"));
    }

    @Override
    protected HystrixFuture<String> run() {
        Promise<String> p = Promise.create();
        new Thread(new Runnable() {

            @Override
            public void run() {
                p.onSuccess("hello");
            }

        }).start();

        return p.createFuture();
    }

    @Override
    protected HystrixFuture<String> getFallback() {
        Promise<String> p = Promise.create();
        p.onSuccess("hello fallback");
        return p.createFuture();
    }

}

[benjchristensen]

Here is observable command usage:

public class ExampleObservableCommand extends HystrixObservableCommand<String> {

    ExampleObservableCommand() {
        super(HystrixCommandGroupKey.Factory.asKey("ExampleObservable"));
    }

    @Override
    protected Observable<String> construct() {
        return Observable.just("hello", "world").subscribeOn(Schedulers.io());
    }

    @Override
    protected Observable<String> onFailureResumeWithFallback() {
        return Observable.just("hello fallback");
    }

}

[bbaetz]

Any reason HystrixObservable doesn't include toObservable(Scheduler)? The copied javadocs reference using HystrixCommand.toObservable(Scheduler).

[bbaetz]

And should there be an interface that implements both HystrixExecutable and HystrixObservable, so that implementations that provide both options (eg HystrixCommand) can be passed around more easily?

[benjchristensen]

Any reason HystrixObservable doesn't include toObservable(Scheduler)? The copied javadocs reference using HystrixCommand.toObservable(Scheduler).

This method has been removed and was only ever intended as an implementation detail anyways.

And should there be an interface that implements both HystrixExecutable and HystrixObservable

What do you suggest calling it?

[bbaetz]

This method has been removed and was only ever intended as an implementation detail anyways.

Hmm, I thought I had a use for it, but I'm possibly misunderstanding something. I'll post to the group with my use case for comment

And should there be an interface that implements both HystrixExecutable and HystrixObservable

What do you suggest calling it?

HystrixRunnable? HystrixInvokable, and rename the marker interface to something else (HystrixCallable?)

[benjchristensen]

@bbaetz You can achieve the same callback scheduling behavior by using the RxJava observeOn operator. The method I removed was just doing that.

[mattrjacobs]

I'd like to consider renaming HystrixAsyncCommand. The HystrixObservableCommand also executes asynchronously. To me, it's important to distinguish that the HystrixAsyncCommand is both scalar and async, and I think the concept of a Future is common enough that naming it HystrixFutureCommand conveys this well.

[mattrjacobs]

On the HystrixAsyncCommand/HystrixFutureCommand, I think a more appropriate name for the action is start(), instead of run(). I draw a parallel to the Thread API, where run() executes synchronously, and start() executes async.

[mattrjacobs]

For HystrixFuture and Promise, is there any value in making them support covariance?

[mattrjacobs]

There's nothing intrinsically Hystrix-y about the HystrixFuture and Promise. Have you considered moving these classes to RxJava? i.e. Are they useful enough constructs that projects that consume only RxJava and not Hystrix could benefit?

[mattrjacobs]

For HystrixObservableCommand, I think resumeWithFallback is a better name than onFailureResumeWithFallback. Once you mention fallback, I think that immediately denotes failure, so putting "failure" in the method name is unnecessary.

[benjchristensen]

HystrixFutureCommand

I like this.

Have you considered moving these classes to RxJava? i.e. Are they useful enough constructs that projects that consume only RxJava and not Hystrix could benefit?

Yes. But I'm far from ready for that -> ReactiveX/RxJava#1594

Due to that not being ready I can't rely on it here. So do we move forward with this interface in Hystrix, or skip HystrixFutureCommand until RxJava provides it?

[benjchristensen]

I think resumeWithFallback is a better name than onFailureResumeWithFallback

I'm fine with the shorter resumeWithFallback.

[benjchristensen]

For HystrixFuture and Promise, is there any value in making them support covariance?

Oh probably.

[benjchristensen]

I think a more appropriate name for the action is start(), instead of run()

I'm good with that.

[benjchristensen]

While reviewing with @KoltonAndrus HystrixExecutableInfo should probably be renamed to HystrixInvokableInfo since HystrixExecutable doesn't apply to all three.

Also discussed HystrixInvokable being HystrixRunnable or HystrixCallable as per ideas from @bbaetz above but decided those imply that the Java Runnable or Callable interfaces are implemented, but they aren't.

[benjchristensen]

While maturing the object model we now have AbstractCommand as the parent of all 3 impls, so we may be able to remove the awkward HystrixExecutableInfo and just reference AbstractCommand.

[benjchristensen]

As per discussion with @KoltonAndrus and @mattrjacobs we will remove HystrixAsyncCommand/HystrixFutureCommand for now until we prove it out more and/or RxJava offers a scalar Observable.