Skip to content

Conversation

@mattrjacobs
Copy link
Contributor

This is a large reorganization of Hystrix internals to support unsubscription.

The changes are:

  • Move cleanup actions to happen on the first of terminate/unsubscribe
  • Wire up Hystrix state machine as a sequence of operators, rather than using explicit Observable.create
  • Be explicit about which state is shared across commands in a request (HystrixCommand.executionResult), and that which is owned by a single command. For example, a command coming from cache, or being cancelled, is only a property of that command, while a SUCCESS is a property of all.

I've added a number of tests to check unsubscription. Outstanding work:

  • Let explicit unsubscription trigger a thread interruption. At the moment, only timeouts trigger thread interruptions, while explicit unsubscription triggers a cancellation of the future without a corresponding interrupt.
  • Add CANCELLED metrics
  • At least one edge case exists for metrics hat isn't handled well at the moment: Command A starts execution, then Command B gets invoked with the same arguments and is supplied by the Observable in Command A. Then, Command A is unsubscribed. The underlying work still goes on, but A's latency stops when it is unsubscribed. Since Command B comes from cache, its latency is 0. So nothing tracks the remainder of the underlying work from when A gets cancelled to its completion.

@mattrjacobs
Copy link
Contributor Author

This addresses #1199 and #220

@stevegury
Copy link

👍
No big deal but a couple of those new Atomic variables could be replaced by volatile variables.

Matt Jacobs added 2 commits May 13, 2016 12:29
@mattrjacobs
Copy link
Contributor Author

Thanks @stevegury. Just modified the state in AbstractCommand to be volatile whenever possible

@stevegury
Copy link

👍

@mmanciop
Copy link

I think the implementation of com.netflix.hystrix.HystrixCommand.queue() is still not compliant with Future.cancel(boolean) as it does not seem to interrupt the execution thread.

@mattrjacobs
Copy link
Contributor Author

I just created a new issue to track making this work properly: #1268

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants