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

Hystrix 1.4 - Async/Non-Blocking [Preview] #209

Merged
merged 37 commits into from
Mar 11, 2014
Merged

Hystrix 1.4 - Async/Non-Blocking [Preview] #209

merged 37 commits into from
Mar 11, 2014

Conversation

benjchristensen
Copy link
Contributor

This is a preview of Hystrix 1.4 that adds HystrixNonBlockingCommand. Thank you to @neerajrj for this work!

This will achieve issue #11.

The intent is to allow bulk-heading, circuit breaker and metrics support around non-blocking network calls. It will not use thread-isolation, only semaphore and timeouts.

Here is a simple example executing a network call via Netty:

public class ObservableCommandWikipediaSearch extends HystrixNonBlockingCommand<String> {

    private final String query;

    protected ObservableCommandWikipediaSearch(String query) {
        super(Setter.withGroupKey(HystrixCommandGroupKey.Factory.asKey("Wikipedia"))
                .andCommandPropertiesDefaults(HystrixCommandProperties.Setter().
                        withExecutionIsolationThreadTimeoutInMilliseconds(4000)));
        this.query = query;
    }

    @Override
    protected Observable<String> run() throws Exception {
        String url = "http://en.wikipedia.org//w/api.php?action=query&generator=search&prop=info&format=json&gsrsearch=" + query;
        return RxNetty.createHttpRequest(url).flatMap(new Func1<ObservableHttpResponse<Object>, Observable<Object>>() {

            @Override
            public Observable<Object> call(ObservableHttpResponse<Object> response) {
                return response.content();
            }

        }).map(new Func1<Object, String>() {

            @Override
            public String call(Object t1) {
                return String.valueOf(t1);
            }

        });
    }

    public static void main(String args[]) {
        HystrixNonBlockingCommand<String> c = new ObservableCommandWikipediaSearch("zebra");
        System.out.println(c.observe().toBlockingObservable().last());
        System.out.println(c.getExecutionTimeInMilliseconds() + "ms " + c.getExecutionEvents());
    }
}

This code is not final nor is the RxNetty library it's using ready, but it demonstrates the behavior of HystrixNonBlockingCommand.

That are likely some things to change still before this is released and I am waiting until RxJava 0.17 (ReactiveX/RxJava#802) is released so Hystrix 1.4 will depend on it.

We have been running this on production canaries for several days to test existing code paths. That does not mean however that the new HystrixNonBlockingCommand is fully tested as our production systems are not yet using it.

Please provide feedback on naming, design, etc.

@cloudbees-pull-request-builder

Hystrix-pull-requests #68 SUCCESS
This pull request looks good

@benjchristensen
Copy link
Contributor Author

@neerajrj I have some questions about NonBlockingCommand:

            private LatchedSemaphoreCommand(TestCircuitBreaker circuitBreaker, TryableSemaphore semaphore,
                    CountDownLatch startLatch, CountDownLatch waitLatch) {
                super(testPropsBuilder().setCircuitBreaker(circuitBreaker).setMetrics(circuitBreaker.metrics)
                        .setCommandPropertiesDefaults(HystrixCommandProperties.Setter.getUnitTestPropertiesSetter().withExecutionIsolationStrategy(ExecutionIsolationStrategy.SEMAPHORE))
                        .setExecutionSemaphore(semaphore));
                this.startLatch = startLatch;
                this.waitLatch = waitLatch;
            }

            @Override
            protected Observable<Boolean> run() {
                // signals caller that run has started
                this.startLatch.countDown();

                try {
                    // waits for caller to countDown latch
                    this.waitLatch.await();
                } catch (InterruptedException e) {
                    e.printStackTrace();
                    return Observable.from(false);
                }
                return Observable.from(true);
            }
        }

This is blocking inside the run() method and therefore probably not testing actual behavior.

For example, in subscribeWithSemaphoreIsolation it is relying upon try/catch/finally which I believe only works because the unit tests is blocking (both with the latches, and the synchronous Observable.from()).

                } catch (Exception e) {
                    /* execution time (must occur before terminal state otherwise a race condition can occur if requested by client) */
                    recordTotalExecutionTime(invocationStartTime);
                    observer.onError(e);
                    // empty subscription since we executed synchronously
                } finally {
                    // pop the command that is being run
                    Hystrix.endCurrentThreadExecutingCommand();
                }

            } finally {
                // release the semaphore
                executionSemaphore.release();
            }

Am I correct that this unit test needs to change and put the latches inside an async Observable rather than inside the run() method?

neerajrj and others added 20 commits February 11, 2014 20:14
- updated unit test that no longer worked with new Scheduler implementation
- they have outgrown the inner class model
- I have made many things package accessible to make tests work
- several are still breaking because they are accessing private members and I have not decided how to handle them
- all unit tests compiling
-  not all are passing as the RxJava 0.17 upgrade is still in process
RxJava 0.17 handles synchronous Subscriptions now so this code needed to be changed otherwise the exceptions never got thrown since the isUnsubscribed() would already be true.
Deriving the full class names for these inner classes was being done wrong and resulted in bad class names when the unit tests got moved.
This reveals the bugs in the current HystrixNonBlockingCommand implementation.
- unit tests are all passing with both async and sync Observables
- defaults to semaphore isolation but allows thread isolation if an Observable source is synchronous
@benjchristensen
Copy link
Contributor Author

HystrixNonBlockingCommand is now HystrixObservableCommand. I believe it is now completely functional with both blocking and non-blocking origin Observables.

- Use "Operator" name as that's what it is.
- Use HystrixObservableTimeoutOperator instead of TimeoutOperator so in a stacktrace it's clear it's the Hystrix variant and not the normal RxJava TimeoutOperator.
- all unit tests are now passing with semaphore and thread isolation
@cloudbees-pull-request-builder

Hystrix-pull-requests #83 FAILURE
Looks like there's a problem with this pull request

@benjchristensen
Copy link
Contributor Author

Just pushed fixes for HystrixObservableCommand.

Work left to do:

  • javadocs and example code
  • finalize and assert the thread model (no timer threads calling onNext, when to use observeOn, etc)
  • production test HystrixCommand
  • development test HystrixObservableCommand

@benjchristensen
Copy link
Contributor Author

Failing test testRequestCacheOnThreadRejectionThrowsException will be fixed with this change in RxJava: ReactiveX/RxJava#924

@benjchristensen
Copy link
Contributor Author

Not quite sure why the HystrixTest unit tests are failing on CloudBees as those are passing locally for me ... hunting down.

@benjchristensen
Copy link
Contributor Author

Grrr, those tests fail when run from gradle, but not from within the IDE.

@cloudbees-pull-request-builder

Hystrix-pull-requests #85 FAILURE
Looks like there's a problem with this pull request

@benjchristensen
Copy link
Contributor Author

Fixed the HystrixTest failures. Now just testRequestCacheOnThreadRejectionThrowsException which will be resolved with the next RxJava release being done shortly.

@cloudbees-pull-request-builder

Hystrix-pull-requests #89 FAILURE
Looks like there's a problem with this pull request

Update tests to assert isExecutedInThread true/false so we are sure the tests are getting what they expect.
@cloudbees-pull-request-builder

Hystrix-pull-requests #90 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

Hystrix-pull-requests #91 FAILURE
Looks like there's a problem with this pull request

@benjchristensen
Copy link
Contributor Author

Javadoc causes the build to fail? Really?

... breaks things (thought nothing would implement this interface, but alas some things do).
@cloudbees-pull-request-builder

Hystrix-pull-requests #92 FAILURE
Looks like there's a problem with this pull request

Fixes issue reported in #212
This was also fixed in 1.3.10
@cloudbees-pull-request-builder

Hystrix-pull-requests #94 FAILURE
Looks like there's a problem with this pull request

@alexwen
Copy link

alexwen commented Mar 5, 2014

with RxJava 0.17 soon upon us, do you have an estimate how far behind Hystrix 1.4 will be? Or will you want to get some bug fix releases off of 0.17 before moving Hystrix over?

@benjchristensen
Copy link
Contributor Author

Theoretically Hystrix 1.4 is functional now and once RxJava 0.17 is released I am open to doing a beta release. I am not 100% certain this code is production ready though and due to the critical nature of this library I don't plan on releasing it officially until I've seen it running in our production environment for a while, and that is likely going to take us a couple weeks to finish doing.

@alexwen
Copy link

alexwen commented Mar 6, 2014

Very understandable. Thanks for the update!

tnine pushed a commit to usergrid/usergrid that referenced this pull request Mar 10, 2014
@benjchristensen benjchristensen merged commit fc99fce into Netflix:master Mar 11, 2014
@benjchristensen benjchristensen deleted the hystrix-1.4-non-blocking branch March 22, 2014 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants