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

Version 1.3 - RxJava Observable Integration #151

Merged
merged 23 commits into from
Jul 26, 2013
Merged

Version 1.3 - RxJava Observable Integration #151

merged 23 commits into from
Jul 26, 2013

Conversation

benjchristensen
Copy link
Contributor

This pull request completes issue #123 "Support Asynchronous Callbacks with RxJava Integration"

Async execution can now be done with observe() and it will callback when the value is received:

Observable<String> s = new CommandHelloWorld("World").observe();

Here is the simplest possible example of subscribing to the value (using a lambda instead of anonymous inner class in this example):

s.subscribe({ value -> println(value) })

More can be learned about RxJava and the composition features at https://github.com/Netflix/RxJava/wiki

This pull request is being canary tested at Netflix and will not be merged or released until we are comfortable with deploying this to our full production traffic. Since this is a non-trivial change I will probably leave it in our canary environment for around a week before proceeding (I have already done several smaller canary tests at various development stages). If any issues are found this pull request will be updated with the fix.

I am posting this pull request before our testing is complete to give everyone a heads up and an opportunity to provide feedback.

benjchristensen and others added 14 commits May 4, 2013 20:46
#123

- refactored HystrixCommand to use Observable inside the implementation to be non-blocking and callback driven
- observe() and toObservable() public methods added
- HystrixCollapser not yet changed
- pick up concurrency fix to ScheduledObserver ReactiveX/RxJava#269
- it is relying on scheduling of the collapser timer
- this is an example so specifically should not be refactored to use artifical time
…ead while other threads working of the Cached wrapper are already waiting on the result
Conflicts:
	gradle.properties
	hystrix-core/src/main/java/com/netflix/hystrix/HystrixCommand.java
@cloudbees-pull-request-builder

Hystrix-pull-requests #22 ABORTED

@benjchristensen
Copy link
Contributor Author

After running on a production canary for a couple days a deadlock occurred, so I've introduced a concurrency bug somewhere. Thus it's going to be a while longer before this is ready ... I'm going to refactor the Collapser code as it has become somewhat unwieldy and difficult to reason through.

@benjchristensen
Copy link
Contributor Author

I need to look at how an Observable handles multiple subscriptions when requestCache is disabled. Right now toObservable will throw an IllegalStateException but once you have the Observable it could be executed multiple times if subscribed to multiple times, and that would cause odd metrics/state and possibly race conditions and lost data.

…plify reasoning

… done while hunting down concurrency bug ...
A short-cut return was before the try/finally. With thread interleaving one thread could get the readLock and never release it and then the execute/shutdown methods would block indefinitely trying to get the writeBlock.

I don't know for sure that this was the bug I've been hunting but the symptoms that could occur from this exactly match what happened.
@cloudbees-pull-request-builder

Hystrix-pull-requests #26 ABORTED

- change logger.error to logger.debug on noisy logs
- improve wording on message where a command has failed and the fallback failed
- logger.error is not needed since everywhere that it would be valuable throws a HystrixRuntimeException which will be handled or logged elsewhere
@cloudbees-pull-request-builder

Hystrix-pull-requests #27 ABORTED

This functionality of aggressive starts from a blocking request has removed for now since we've moved to a completely async model.

If testing shows we still need/want this for performance optimizations we can add it back in.
@cloudbees-pull-request-builder

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

@cloudbees-pull-request-builder

Hystrix-pull-requests #29 ABORTED

- added Javadoc to formalize this contract (it was always there)
- reverted back to 1.2 behavior where IllegalStateException is thrown directly, not wrapped inside a HystrixRuntimeException
@cloudbees-pull-request-builder

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

@cloudbees-pull-request-builder

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

@cloudbees-pull-request-builder

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

@benjchristensen
Copy link
Contributor Author

Testing has looked good in production canaries for the past 2 days so merging this into trunk.

Will then proceed with a larger scale canary test in Netflix production with binaries created through the formal build process. I will not release that to Maven Central until I have validated further.

benjchristensen added a commit that referenced this pull request Jul 26, 2013
Version 1.3 - RxJava Observable Integration
@benjchristensen benjchristensen merged commit 4feab8a into Netflix:master Jul 26, 2013
@benjchristensen benjchristensen deleted the version_1_3-rxjava 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.

None yet

3 participants