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

Execution Hook differences for HystrixCommand/HystrixObservableCommand #682

Closed
mattrjacobs opened this issue Feb 14, 2015 · 3 comments
Closed
Milestone

Comments

@mattrjacobs
Copy link
Contributor

Ran into this while working on implementing #600
Currently, HystrixCommandExecutionHook has the following methods (among others):

  • onRunStart(HystrixInvokable<T> instance)
  • onRunSuccess(HystrixInvokable<T> instance, T response)
  • onRunError(HystrixInvokable<T> instance, Exception ex)
  • onStart(HystrixInvokable<T> instance)
  • onComplete(HystrixInvokable<T> instance, T response)
  • onError(HystrixInvokable<T> instance, FailureType failureType, Exception ex)

This set of methods matches well to HystrixCommand, but not HystrixObservableCommand.

  • HystrixCommand has a run() method, HystrixObservableCommand has construct(), so the naming feels strange
  • onRunSuccess conflates an emission (it contains the response) and a terminal signal. HystrixObservableCommand may emit many values before the overall command is marked as a 'success' (an onCompleted)
  • onComplete also suffers this conflation

\cc @benjchristensen for thoughts on design

@mattrjacobs mattrjacobs added this to the 1.4.0-RC8 milestone Feb 14, 2015
@mattrjacobs
Copy link
Contributor Author

Here are my thoughts on how to handle this mismatch:

  1. Deprecate onRunStart, onRunSuccess, onRunError because the name is misleading. Keep their behavior the same
  2. Deprecate onFallbackSuccess, onComplete because they include a value. This is only appropriate for the single-valued case
  3. This is the new set of un-deprecated hooks (new are bolded):
    • onStart(HystrixInvokable<T> commandInstance)
    • onEmit(HystrixInvokable<T> commandInstance, T value)
    • onError(HystrixInvokable<T> commandInstance, FailureType failureType, Exception e)
    • onSuccess(HystrixInvokable<T> commandInstance)
    • onThreadStart(HystrixInvokable<T> commandInstance)
    • onThreadComplete(HystrixInvokable<T> commandInstance)
    • onExecutionStart(HystrixInvokable<T> commandInstance)
    • onExecutionEmit(HystrixInvokable<T> commandInstance, T value)
    • onExecutionError(HystrixInvokable<T> commandInstance, Exception e)
    • onExecutionSuccess(HystrixInvokable<T> commandInstance)
    • onFallbackStart(HystrixInvokable<T> commandInstance)
    • onFallbackEmit(HystrixInvokable<T> commandInstance, T value)
    • onFallbackError(HystrixInvokable<T> commandInstance, Exception e)
    • onFallbackSuccess(HystrixInvokable<T> commandInstance)
    • onCacheHit(HystrixInvokable<T> commandInstance)

@benjchristensen
Copy link
Contributor

I think that's good.

@mattrjacobs
Copy link
Contributor Author

Completed in #683

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

No branches or pull requests

2 participants