search

Comcast / jrugged

A Java libary of robustness design patterns
Apache License 2.0
268 stars 94 forks source link

Support asynchronous method calls #41

Closed christianapel closed 8 years ago

christianapel commented 9 years ago

Introducing Spring 4 based CircuitBreaker and RequestCounter to allow asynchronous method calls by using ListenableFutures.

christianapel commented 9 years ago

Please note that this pull request contains the bugfix commit of pull request #40.

christianapel commented 9 years ago

Servlet 3.0 couldn't be resolved by the Travis build, but javax.servlet.AsyncContext is required at runtime.

christianapel commented 9 years ago

@joercampbell Have you already had a chance to take a look at this pull request?

Since the current jrugged modules doesn't support asynchronous calls, I thought about implementing it without big changes in the core module. By updating the Spring version in the jrugged-spring module and extending three classes of the core module, an optional asynchronous API could be provided with minimal core module updates. The jrugged-spring module has a Spring dependency anyway, therefore I have used the ListenableFuture interface to asynchronously wrap a service call within the circuit breaker logic.

Please let me know what you think about this change, or if you have any questions about it.

joercampbell commented 9 years ago

I started looking at it yesterday when I was doing the other pull request work. I'll get more time to look at it starting Thursday as I am at a conference today all day.

Thanks for the submission.

Joe On Apr 8, 2015 6:58 AM, "Christian Apel" notifications@github.com wrote:

@joercampbell https://github.com/joercampbell Have you already had a chance to take a look at this pull request?

Since the current jrugged modules doesn't support asynchronous calls, I thought about implementing it without big changes in the core module. By updating the Spring version in the jrugged-spring module and extending three classes of the core module, an optional asynchronous API could be provided with minimal core module updates. The jrugged-spring module has a Spring dependency anyway, therefore I have used the ListenableFuture interface to asynchronously wrap a service call within the circuit breaker logic.

Please let me know what you think about this change, or if you have any questions about it.

— Reply to this email directly or view it on GitHub https://github.com/Comcast/jrugged/pull/41#issuecomment-90880055.

joercampbell commented 9 years ago

Two small-ish questions:

1) Would you be ok if I renamed the items in the spring group to AsyncCircuitBreaker etc? I would like to be able to distinguish those things and their spring/async nature from the existing implementation.

2) Do you happen to have any unit/functional tests to go along with their implementation?

I am curious as well - having this be async has the possibility of essentially queuing circuitbreaker failures within the system someone would be building. In the fail fast sense - wouldn't you still need to inspect immediately any response to see if the response was an error'ed (Breaker open) state?

christianapel commented 9 years ago

1) I've also thought about the names and then decided to remove the 'Async' prefix from the class names. But I'm also okay with AsyncCircuitBreaker, so feel free to rename the classes.

2) No, unfortunately I don't have any unit or functional tests. We have updated our existing implementation which is internally using your circuitbreaker to use the new async classes. We have then performed manual tests and used our integration tests to verify that the asynchronous methods are working as expected.

Regarding your concerns about the fail fast sense, it should still work like in the synchronous code. Before an asynchronous call is triggered, the breaker state is checked and an OPEN breaker would immediately reject the call. When an asynchronous call is actually dispatched, the breaker state is updated if a failure is returned and therefore the behavior should also match with the synchronous variant.

christianapel commented 9 years ago

Any updates regarding this pull request? Do you have any further questions or can I help you with something?

joercampbell commented 9 years ago

I merged it to trunk, but I took it in two pieces. Sorry that it took so long, but I needed to work out some small details with Comcast (a primary supporter of this library) and myself as I am no longer an employee there. The primary of which is a CLA (Contributor document) which at some point, everyone contributing to this project will have to also fill out. Its very similar to a contribution to an apache project in that sense.

I merged to master - upped to version 4.0 because the versions of some of the dependencies changed (notably the spring version). I want to see about getting some unit tests written for this before I officially publish it into maven central as version 4.0. Any help you want to provide there would be of course appreciated - as is your continued support of this library. :)

Thanks.