Execution Hooks via Plugin

[benjchristensen]

Create an ExecutionHook strategy that can be implemented to do things such as:

• decorate a response
• conditionally throw exceptions, modify responses
• do other forms of logging or diagnostics
• be invoked ...
  • before run()
  • start of run()
  • end of run()
  • after run()
  • start of getFallback()
  • end of getFallback()
  • etc

[benjchristensen]

Here is the start of what the interface could look like.

I would like to review this with a few people before proceeding much further to be sure it accomplishes the desired goals and I'm not missing something.

/**
 * Abstract ExecutionHook with invocations at different lifecycle points of {@link HystrixCommand} execution with default no-op implementations.
 * <p>
 * See {@link HystrixPlugins} or the Hystrix GitHub Wiki for information on configuring plugins: <a
 * href="https://github.com/Netflix/Hystrix/wiki/Plugins">https://github.com/Netflix/Hystrix/wiki/Plugins</a>.
 * */
public abstract class HystrixCommandExecutionHook {

    /**
     * Invoked before {@link HystrixCommand#run()} is about to be executed.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     */
    public <T> void startRun(HystrixCommand<T> commandInstance) {
        // do nothing by default
    }

    /**
     * Invoked after successful execution of {@link HystrixCommand#run()} with response value.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     * @param response
     *            from {@link HystrixCommand#run()}
     * @return T response object that can be modified, decorated, replaced or just returned as a pass-thru.
     */
    public <T> T endRunSuccess(HystrixCommand<T> commandInstance, T response) {
        // pass-thru by default
        return response;
    }

    /**
     * Invoked after failed execution of {@link HystrixCommand#run()} with thrown Exception.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     * @param e
     *            Exception thrown by {@link HystrixCommand#run()}
     */
    public <T> void endRunFailure(HystrixCommand<T> commandInstance, Exception e) {
        // do nothing by default
    }

    /**
     * Invoked before {@link HystrixCommand#getFallback()} is about to be executed.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     */
    public <T> void startFallback(HystrixCommand<T> commandInstance) {
        // do nothing by default
    }

    /**
     * Invoked after successful execution of {@link HystrixCommand#getFallback()} with response value.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     * @param fallbackResponse
     *            from {@link HystrixCommand#getFallback()}
     * @return T response object that can be modified, decorated, replaced or just returned as a pass-thru.
     */
    public <T> T endFallbackSuccess(HystrixCommand<T> commandInstance, T fallbackResponse) {
        // pass-thru by default
        return fallbackResponse;
    }

    /**
     * Invoked after failed execution of {@link HystrixCommand#getFallback()} with thrown exception.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     * @param e
     *            Exception thrown by {@link HystrixCommand#getFallback()}
     */
    public <T> void endFallbackFailure(HystrixCommand<T> commandInstance, Exception e) {
        // do nothing by default
    }

    /**
     * Invoked before {@link HystrixCommand#execute()} executes the command.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     */
    public <T> void startExecute(HystrixCommand<T> commandInstance) {
        // do nothing by default
    }

    /**
     * Invoked after {@link HystrixCommand#execute()} successfully executes the command.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     * @param response
     *            from {@link HystrixCommand#execute()}
     * @return T response object that can be modified, decorated, replaced or just returned as a pass-thru.
     */
    public <T> T endExecuteSuccess(HystrixCommand<T> commandInstance, T response) {
        // pass-thru by default
        return response;
    }

    /**
     * Invoked after failed execution of {@link HystrixCommand#execute()} with thrown Exception.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     * @param e
     *            Exception thrown by {@link HystrixCommand#execute()}
     */
    public <T> void endExecuteFailure(HystrixCommand<T> commandInstance, Exception e) {
        // do nothing by default
    }

    /**
     * Invoked before {@link HystrixCommand#queue()} queues the command for execution.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     */
    public <T> void startQueue(HystrixCommand<T> commandInstance) {
        // do nothing by default
    }

    /**
     * Invoked after successful completion of Future from {@link HystrixCommand#queue()} execution.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     * @param response
     *            from {@link HystrixCommand#queue().get()}
     * @return T response object that can be modified, decorated, replaced or just returned as a pass-thru.
     */
    public <T> T endQueueSuccess(HystrixCommand<T> commandInstance, T response) {
        // pass-thru by default
        return response;
    }

    /**
     * Invoked after failed completion of Future from {@link HystrixCommand#queue()} execution.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     * @param e
     *            Exception thrown by {@link HystrixCommand#queue().get()}
     */
    public <T> void endQueueFailure(HystrixCommand<T> commandInstance, Exception e) {
        // do nothing by default
    }

}

[neerajrj]

Looks good to me.
This may depend on how users plan to use the hooks but if someone wants a hook at endExecute (regardless of success or failure) they would have to add a method that gets called from both the endExecuteSuccess and endExecuteFailure hooks or something like that.
In the past I have seen these hooks to just indicate start and end (regardless of success/failure) but it seems like this is a stylistic thing more than anything.

[benjchristensen]

I considered that type of method signature, like this:

    public <T> T endQueue(HystrixCommand<T> commandInstance, T response, Exception e) {
        // pass-thru by default
        return response;
    }

If it was just for notification then it would work well and someone can just check for null on the exception.

However, it makes the response value awkward for success since a response value isn't needed for a failure.

The idea is that on a success, the response value will be passed in and then could be decorated, modified or replaced and returned.

I'm even considering doing the same for error so that the Exception can be returned so if someone wanted to they could decorate an exception.

[benjchristensen]

For example, I'm considering this:

    /**
     * Invoked after successful completion of Future from {@link HystrixCommand#queue()} execution.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     * @param response
     *            from {@link HystrixCommand#queue().get()}
     * @return T response object that can be modified, decorated, replaced or just returned as a pass-thru.
     */
    public <T> T endQueueSuccess(HystrixCommand<T> commandInstance, T response) {
        // pass-thru by default
        return response;
    }

    /**
     * Invoked after failed completion of Future from {@link HystrixCommand#queue()} execution.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     * @param e
     *            Exception thrown by {@link HystrixCommand#queue().get()}
     * @return Exception that can be modified, decorated, replaced or just returned as a pass-thru.
     */
    public <T> Exception endQueueFailure(HystrixCommand<T> commandInstance, Exception e) {
        // pass-thru by default
        return e;
    }

[mhawthorne]

I think this looks good.

I agree with allowing the failure hooks to decorate (or replace) the thrown exception, to make them more consistent with the other methods.

One question: what is the exception that will be passed to endRunFailure when an execution is short circuited?

[benjchristensen]

The startRun and endRun methods would never be called if it is short-circuited as the run() method is never invoked.

[benjchristensen]

Based on the conversations I've had here and in-person I'm proceeding with this pattern:

    /**
     * Invoked before {@link HystrixCommand#run()} is about to be executed.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     */
    public <T> void startRun(HystrixCommand<T> commandInstance) {
        // do nothing by default
    }

    /**
     * Invoked after successful execution of {@link HystrixCommand#run()} with response value.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     * @param response
     *            from {@link HystrixCommand#run()}
     * @return T response object that can be modified, decorated, replaced or just returned as a pass-thru.
     */
    public <T> T endRunSuccess(HystrixCommand<T> commandInstance, T response) {
        // pass-thru by default
        return response;
    }

    /**
     * Invoked after failed execution of {@link HystrixCommand#run()} with thrown Exception.
     * 
     * @param commandInstance
     *            The executing HystrixCommand instance.
     * @param e
     *            Exception thrown by {@link HystrixCommand#run()}
     * @return Exception that can be decorated, replaced or just returned as a pass-thru.
     */
    public <T> Exception endRunFailure(HystrixCommand<T> commandInstance, Exception e) {
        // pass-thru by default
        return e;
    }

[codefromthecrypt]

should failures declare Throwable instead of Exception?

[benjchristensen]

I was specifically using Exception instead of Throwable since my understanding is that it's generally bad practice to be catching Throwable generically.

There should be just Exception and Error extending from Throwable and we don't want to catch Error.

Per the JDK it is generally not reasonable for an application to generically catch Error:

An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch. Most such errors are abnormal conditions. The ThreadDeath error, though a "normal" condition, is also a subclass of Error because most applications should not try to catch it.

Whereas Exception states:

The class Exception and its subclasses are a form of Throwable that indicates conditions that a reasonable application might want to catch.

I'm not aware of any time a Throwable would be thrown that isn't either an Exception or Error that catching Exception makes sense so as to avoid catching Error.

Is there a good reason to catch Throwable that I'm missing?

[codefromthecrypt]

I suppose main thing is that you can at least log or send an event on
Error, even if you immediately propagate it. So basically regardless of
whether the command was killed by an Error, Exception, or RuntimeException,
we could understand what happened, if the field is throwable, and we could
use something like guava Throwables class to smartly propagate:
http://code.google.com/p/guava-libraries/wiki/ThrowablesExplained

[benjchristensen]

Error would always propagate out of Hystrix since we don't catch it.

Are there times when a Throwable is thrown that is not either Exception or Error?

[codefromthecrypt]

yeah I'm not aware of any other implementations of Throwable besides Error
and Exception. I suppose you are right, the only way this would be useful
is if we internally caught Throwable. Otherwise, it would always be
Exception or a subtype registered. Only issue is that since these are
parameters, if there was a desire to change the interface later to accept
throwable, we'd have the same-arity overload stuff. If we are sure Hystrix
doesn't ever want to alert about error conditions that stopped commands,
then def we are safe and clean to use Exception as the param. Assuming
this is the case, we'd just make a javadoc note that the implementation
contract of HystrixCommand implies errors propagate and so the event model
will miss these failures.

[benjchristensen]

Yes, that is the question: should Hystrix catch Error/Throwable or only Exception?

My view has been that it should only catch Exception (though I mistakenly had it catching Throwables up until recently in some places).

[neerajrj]

IMO catching Throwables should be reserved for very special cases. I think if a user really needs to catch it they can do so themselves and rethrow it as a typed exception as an example.

[benjchristensen]

A potential use case that @opuneet just came up with for debugging is that when a separate thread is running (that Hystrix executes) and if it has an Error it will not propagate in a way the application can catch.

We should at minimum log that but perhaps that's the use case that warrants this using Throwable.

[codefromthecrypt]

I think that the key here is that this interface is a reflection of what information hystrix has about certain events. For example, this doesn't imply that users of hystrix catch throwables. It only means that hystrix logs when they occur via this interface. IMHO, it is ok for Hystrix to catch Throwable for the purpose of sending notification that a command failed even if it immediately propagates it.

[benjchristensen]

I've stuck with Exception instead of Throwable as Hystrix is only handling Exceptions.

The thing that finished convincing me to stick with Exception is I can't throw a Throwable from inside the child thread since Callable only throws Exception.

http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Callable.html

The interfaces will stay with Exception. If we find places where we want to catch Throwable and log Error types we can do so as implementations details, but not via event hooks or notification.

[benjchristensen]

If any of you are interested in doing a code review I'm going to leave this pull request until tomorrow (or until Monday if I don't get around to it tomorrow) before merging it.

The commits in particular for the execution hook changes are https://github.com/benjchristensen/Hystrix/commit/390026a82d0b46c4524667c974581c0dd1a5e93e and https://github.com/benjchristensen/Hystrix/commit/aedb7962f8c533c97ef5526c9f2ad1b3324a0724

[codefromthecrypt]

Keep in mind that callable is an implementation detail. Ex, if I use nio
or otherwise to create a future, callable may never arise. Further the
cause of ExecutionException is a throwable.

Another note is that this interface could be alternatively done as an event
type hierarchy + observer pattern. Ex. I've done similar with guava
eventbus. Thoughts?

On Jan 4, 2013 9:37 PM, "Ben Christensen" notifications@github.com wrote:

I've stuck with Exception instead of Throwable as Hystrix is only
handling Exceptions.

The thing that finished convincing me to stick with Exception is I can't
throw a Throwable from inside the child thread since Callable only throws
Exception.

http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Callable.html

The interfaces will stay with Exception. If we find places where we want
to catch Throwable and log Error types we can do so as implementations
details, but not via event hooks or notification.

—
Reply to this email directly or view it on GitHub.

[codefromthecrypt]

Actually, I guess we can always wrap in ExecutionException. Wdyt about
declaring this as the type?

[codefromthecrypt]

In other words, Exception sounds good enough, and impl note is to wrap
throwables if you want folks to be able to consume them.

On topic of whether this should be an interface or an event hierarchy, ill
let you think about it :) regardless, I can create a hierarchy using this
interface, so we can play with the event idea even with current approach.

[benjchristensen]

Throwable/Exception

Based on my reading of the JDK and opinions shared with me by a few people in and beyond this thread I don't think Throwable/Error should be explicitly handled by Hystrix and that Exception is the lowest level that a library should generally concern itself with.

I think Hystrix should ensure that any Throwable/Error thrown within child threads or other asynchronous execution (such as NIO) are propagated and not hidden/swallowed, but beyond that I don't think Hystrix should get involved and just throw them up higher.

Further discussion about propagation of Throwable/Error should be taken up in a different issue I think. I created an issue for the continued discussion: #72

The public interfaces should I believe remain as Exception and not Throwable, following the pattern and guidance of the JDK and as demonstrated by the Callable interface.

Event Propagation

Events could be further propagated using an eventbus, observer pattern or other mechanisms but that is beyond the scope of what I intend for this ExecutionHook interface which was meant as a no-op abstract class that someone can use to implement functionality such as what you reference.

Hystrix should remain agnostic as to what type of event propagation system someone may want to use the same way it is agnostic regarding publishing metrics and currently has implementations for both Servo and Yammer Metrics.

I do not want to make Guava a dependency of Hystrix but it sounds like it would make for a good hystrix-contrib module that implements the ExecutionHook and uses Guava EventBus to propagate events.

[codefromthecrypt]

Thanks for thinking through this.

I often refer to implementations of patterns as an example, not to suggest
we only bind to that implementation. For example, guava's EventBus as an
example of a system that implements observer/observable. I'm not
suggesting this should become a dependency of hystrix, rather that we can
take into account a potential consumer of these events which makes the
discussion more concrete.

Back to the discussion... the interface being discussed declares methods
that correspond to an event hierarchy. My suggestion is rather than create
a wrapper interface, create an event hierarchy and an interface that takes
this into account. This would allow folks to have a simpler interface and
use the java type system as opposed to having to consume a dozen methods
and change them if ever a new event is declared.

ex. QueueEvent which has subclasses of QueueStartedEvent and
QueueFailedEvent.

The interface in hystrix then offer up a much simpler and less brittle
interface:

void handle(HystrixEvent event);

thoughts?

[benjchristensen]

That type of event notification is basically what HystrixEventNotifier is for: https://github.com/Netflix/Hystrix/blob/master/hystrix-core/src/main/java/com/netflix/hystrix/strategy/eventnotifier/HystrixEventNotifier.java

This ExecutionHook has different semantics because it allows throwing exceptions and returning responses or exceptions, thus the method signatures are different.

Here is the current method signature:

public void onRunStart(HystrixCommand commandInstance)
public T onRunSuccess(HystrixCommand commandInstance, T response)
public Exception onRunError(HystrixCommand commandInstance, Exception e)

public void onFallbackStart(HystrixCommand commandInstance)
public T onFallbackSuccess(HystrixCommand commandInstance, T fallbackResponse)
public Exception onFallbackError(HystrixCommand commandInstance, Exception e)

public void onStart(HystrixCommand commandInstance)
public T onComplete(HystrixCommand commandInstance, T response)
public Exception onError(HystrixCommand commandInstance, FailureType failureType, Exception e)

public void onThreadStart(HystrixCommand commandInstance)
public void onThreadComplete(HystrixCommand commandInstance)

You could collapse this to:

/* success here actually means success, not complete with success or fallback */
public void onHookStart(HookType type, HystrixCommand commandInstance)
public T onHookSuccess(HookType type, HystrixCommand commandInstance, T response)
public Exception onHookError(HookType type, HystrixCommand commandInstance, Exception e)

/* complete here means it came from either run() or fallback() */
public void onStart(HystrixCommand commandInstance)
public T onComplete(HystrixCommand commandInstance, T response)
public Exception onError(HystrixCommand commandInstance, FailureType failureType, Exception e)

/* thread behavior is different than the others so only has start/complete without return values */
public void onThreadStart(HystrixCommand commandInstance)
public void onThreadComplete(HystrixCommand commandInstance)

The success/error methods can't be collapsed because their return types are different.

It could however go to the following:

public void onStart(HookType type, HystrixCommand commandInstance)
public T onComplete(HookType type, HystrixCommand commandInstance, T response)
public Exception onError(HookType type, HystrixCommand commandInstance, Exception e)

This could handle the current use cases, but the HookType passed in would then need Javadocs explaining specifically how each type is different (complete in some cases means success, in other cases it means failure with a fallback, in another it just means a thread finished).

It also means that every implementation now must embed conditional or switch logic following what the documentation states.

Since the HystrixCommandExecutionHook is an abstract class new methods can be added without breaking backwards compatibility, the same as having new HookTypes be added.

[codefromthecrypt]

OK. I see what you mean. From the proposals, I definitely prefer the
latter syntax, with a light preference of using the word Callback as
opposed to Hook.

[codefromthecrypt]

Actually, maybe the best name for this is interceptor, as it is implied
that the implementor can mutate the response, and their code can completely
screw the command. Similar to aop.

http://aopalliance.sourceforge.net/doc/org/aopalliance/intercept/MethodInterceptor.html

Thoughts?

[benjchristensen]

It is similar to AOP in the sense that it can intercept a call.

The name "execution hook" came from other uses of the "hook" term such as:

http://git-scm.com/docs/githooks
http://ellislab.com/codeigniter/user-guide/general/hooks.html
http://www.webhooks.org
https://github.com/blog/964-all-of-the-hooks
https://help.github.com/articles/post-receive-hooks

[benjchristensen]

I was wrong before about collapsing to the 3 methods. There are different method signatures that got lost.

It would actually need to be these 5 due to different method signatures serving the different use cases that don't apply to each other generically:

public void onStart(HookType type, HystrixCommand commandInstance)
public Exception onError(HookType type, HystrixCommand commandInstance, Exception e)
public Exception onError(HookType type, HystrixCommand commandInstance, FailureType failureType, Exception e)
public T onComplete(HookType type, HystrixCommand commandInstance, T response)
public T onComplete(HookType type, HystrixCommand commandInstance)

The purpose of these hooks is not for generic event notification - we already have that in HystrixEventNotifier and can mature that further if needed.

Here's an example of how the above methods are not truly generic for the different uses:

Type RUN would call:

public void onStart(HookType type, HystrixCommand commandInstance)
public Exception onError(HookType type, HystrixCommand commandInstance, Exception e)
// onComplete will receive a response only in SUCCESSFUL execution
public T onComplete(HookType type, HystrixCommand commandInstance, T response)

Type FALLBACK would call:

public void onStart(HookType type, HystrixCommand commandInstance)
public Exception onError(HookType type, HystrixCommand commandInstance, Exception e)
// onComplete will receive a response only in SUCCESSFUL execution
public T onComplete(HookType type, HystrixCommand commandInstance, T response)

Type COMMAND would call:

public void onStart(HookType type, HystrixCommand commandInstance)
public Exception onError(HookType type, HystrixCommand commandInstance, FailureType failureType, Exception e)
// onComplete will receive a response in SUCCESSFUL or FALLBACK states
public T onComplete(HookType type, HystrixCommand commandInstance, T response)


Type THREAD_EXECUTION would call:

public void onStart(HookType type, HystrixCommand commandInstance)
public T onComplete(HookType type, HystrixCommand commandInstance)

This is rather confusing to try and have generic methods that can't actually be used generically.

We could forcefully make 3 "generic" methods with overloaded arguments and conditionally leave certain arguments NULL but that is awkward and non-obvious and requires a developer to carefully read the documentation and understand how the arguments related to each TYPE.

The implementation of each of the 3 generic methods would in turn have to do conditional logic to handle the different types as they won't be handled the same, will have different arguments passed in and serve different purposes, so it doesn't simplify implementation to have less methods, the code just ends up in conditional logic instead.

I suggest that for the use case this is trying to solve, methods are the right level of abstraction so that each method serves the correct use case and its signature communicates clearly what it does and receives.

• Some hooks allow intercepting and mutating responses or exceptions.
• Some allow throwing exceptions to inject failure.
• Some just get called (such as the onThread methods) to allow for fault injection, logging or modification/inspection of thread state without ability to mutate return values.

As new use cases arise, even those with different method signatures, they can easily be added without breaking existing implementations.

// hooks for run() execution
public void onRunStart(HystrixCommand commandInstance)
public T onRunSuccess(HystrixCommand commandInstance, T response)
public Exception onRunError(HystrixCommand commandInstance, Exception e)

// hooks for getFallback() execution
public void onFallbackStart(HystrixCommand commandInstance) 
public T onFallbackSuccess(HystrixCommand commandInstance, T fallbackResponse)
public Exception onFallbackError(HystrixCommand commandInstance, Exception e)

// hooks for command end-to-end execution
public void onStart(HystrixCommand commandInstance)
public T onComplete(HystrixCommand commandInstance, T response)
public Exception onError(HystrixCommand commandInstance, FailureType failureType, Exception e)

// hooks for child thread execution when using thread isolation
public void onThreadStart(HystrixCommand commandInstance)
public void onThreadComplete(HystrixCommand commandInstance)

[codefromthecrypt]

I suppose my current thoughts are on the fact that this interface includes
a combination of methods safe to call out of band (hooks) as well ones in
the line of fire (interceptors). As an implementor, I'd prefer single
responsibility vs convenience of all things hook-like being in the same
interface. That said, I think you've probably heard plenty enough of
unsolicited advice, and have an interface you need to complete. I
appreciate your sharing the decision rationale and best luck on this one!

[benjchristensen]

I appreciate the feedback ... that's why I asked!

You bring up a valid point about interceptor vs hooks and "single responsibility" vs "convenience".

All of these methods we've been discussing are of an "interceptor" approach, even the onThread* methods without return values or the onStart methods.

The onThread* methods are intended for allowing the plugin implementation to inspect and modify child thread state - such as ThreadLocals (Netflix has a use case for this that was a driver to me implementing this feature).

The onStart* methods can be used to inject failure by throwing exceptions before any work is attempted or inspect/modify parent thread state.

Neither of these feel right in the HystrixEventNotifier which is why I have them here.

I have considered HystrixEventNotifier to be a fire-and-forget style global event notifier serving all of Hystrix, whereas this ExecutorHook class will serve only the HystrixCommand object.

There will eventually be a separate one for HystrixCollapser with very different hooks, such as for batching and sharding and eventually when there is a HystrixAsyncCommand (or whatever it gets called) it would also have something similar.

Hence we end up with the following:

public abstract class HystrixEventNotifier {
    public void markEvent(HystrixEventType eventType, HystrixCommandKey key);
    public void markEvent(HystrixEventType eventType, HystrixCollapserKey key);
    public void markCommandExecution(HystrixCommandKey key, ExecutionIsolationStrategy isolationStrategy, int duration, List<HystrixEventType> eventsDuringExecution);
}

public abstract class HystrixCommandExecutionHook {
    // hooks for run() execution
    public void onRunStart(HystrixCommand commandInstance)
    public T onRunSuccess(HystrixCommand commandInstance, T response)
    public Exception onRunError(HystrixCommand commandInstance, Exception e)

    // hooks for getFallback() execution
    public void onFallbackStart(HystrixCommand commandInstance) 
    public T onFallbackSuccess(HystrixCommand commandInstance, T fallbackResponse)
    public Exception onFallbackError(HystrixCommand commandInstance, Exception e)

    // hooks for command end-to-end execution
    public void onStart(HystrixCommand commandInstance)
    public T onComplete(HystrixCommand commandInstance, T response)
    public Exception onError(HystrixCommand commandInstance, FailureType failureType, Exception e)

    // hooks for child thread execution when using thread isolation
    public void onThreadStart(HystrixCommand commandInstance)
    public void onThreadComplete(HystrixCommand commandInstance)
}

public abstract class HystrixCollapserExecutionHook {

   // hooks for batching

    // hooks for sharding

    // hooks for batch command execution

    // hooks for mapping response back to request

    // hooks for collapser end-to-end execution
    public void onStart(HystrixCommand commandInstance)
    public T onComplete(HystrixCommand commandInstance, T response)
    public Exception onError(HystrixCommand commandInstance, FailureType failureType, Exception e)
}

public abstract class HystrixAsyncCommandExecutionHook {
   // more hooks 
}

I have been thinking about the "single responsibility" being at the level of which object is being targeted: HystrixCommand, HystrixCollapser, etc vs global fire-and-forget event notifications on HystrixEventNotifier.

Perhaps the EventNotifier and ExecutionHook functionality should be merged and I shouldn't be trying to keep them separate? Perhaps the differences between them aren't strong enough to keep them separate?

[codefromthecrypt]

I think I realize how we got off the same page wrt what single-responsibilty means :P So, looking at the PR, it looks like the *Hook pattern is used in the ctors of the commands, set via the staticy thing this.executionHook = HystrixPlugins.getInstance().getCommandExecutionHook(); I suppose that this granularity is helpful to keep the staticy thing from having a million members.

However, maybe a builder could help. IOW unless we expect everyone to override all things in a Hook aggregator, you could reduce the responsibility (also need for extending abstract classes) by making a builder. The smaller classes will be much easier to unit test.

// please note Function doesn't need to be a guava interface, you can make your own
// thing that has apply from, to
enum TweakThreadLocalsOnStarts extends Function<HystrixCommand, Void> {
   INSTANCE; //singleton enum pattern

   public Void apply(HystrixCommand in) {
      //me tweaks
      return null;
   }
}

HystrixAsyncCommandExecutionHookBuilder.newBuilder()
   .onStart(TweakThreadLocalsOnStarts.INSTANCE)
   .build();

[codefromthecrypt]

yeah, and the above could always be added as a different pull request, as it doesn't change the impl details of how *Hooks are used, rather how they are constructed

[mcacker]

Ben, I think the abstract class HystrixCommandExecutionHook looks fine. Any reason why it's an abstract class as opposed to an interface? Not shown here is the method to inject the hook into the command, but I presume that will be pretty straightforward. Could it be different for each command, or a single one injected as a plugin?

It looks like you dropped the idea of a HookType, which does keep things simpler in my mind.

[benjchristensen]

That pattern could be applied but I don't see the choice of overriding or not overriding a method on the abstract class as currently implemented problematic enough to warrant this change, particularly since the addition of a hook is only done once for a system at a global level, so it's not code that will be implemented, configured or changed often unlike the fluent interface used on the HystrixCommand constructors where complex combinations of arguments are potentially coded on every command implementation.

As you say, someone could implement a builder pattern on top of the base abstract class if desired so current implementation doesn't prevent this builder being added going forward.

I'm going to proceed with merging the pull request which is the simplest possible implementation and see over time if usage patterns require anything more advanced. Worst case scenario we deprecate this and add a new improved one - but with real world knowledge of usage patterns to drive the design.

[codefromthecrypt]

sgtm

[benjchristensen]

@mcacker, I am using an abstract class rather than interface so that new methods can be added over time without breaking anyone who has implemented an interface.

It's an approach I learned in past libraries where I had public interfaces that I couldn't change because of existing implementations. In Java 8 this problem will go away since interfaces can have default method implementations.

You can see more about the "Abstract vs Interface" design decision on this page: https://github.com/Netflix/Hystrix/wiki/Plugins

As for lacking an injection per command - my intention was that this plugin is a single implementation that all commands will invoke. It will be registered once globally for a runtime using HystrixPlugins.registerCommandExecutionHook(HystrixCommandExecutionHook impl) or using a system property.

If an implementation wanted to have different behavior for different command types it could treat the base implementation as a controller that conditionally invokes different logic based on the HystrixCommand object passed into each call.

I chose this approach as it is simple and lightweight while allowing implementations to apply logic either globally to all commands or conditionally to only certain commands.

In fact, a complex rules engine could be built on top of this single global abstract class that dynamically invokes classes with logic per-command. One such implementation envisioned is a fault-injection system that would allow conditionally injecting failure into specific commands based on various rules.

[benjchristensen]

Thanks @adriancole for all of your feedback and insight.

[benjchristensen]

Pull Request #71 merged.

[mcacker]

Ben, i'm seeing onComplete called twice for a fallback success. Firstly from HystrixCommand, line 707

        return executionHook.onComplete(_this, r);

Secondly from HystrixCommand, line 1104

        return executionHook.onComplete(this, fallback);

Note that the fallback is itself a Hystrix command, for which I ignore the onComplete method, but the onComplete method is getting called for the Initial command.

Is this to be expected?

thanks, Mitchell

[benjchristensen]

Nested command execution does not change how commands are executed. They each execute independently with their own state.

So you should see the following onComplete invocations for your described use case:

• FallbackCommand.onComplete via line 707
• NormalCommand.onComplete via line 1107

The onComplete method should be invoked once per command if a response value is returned, whether from run() or getFallback().

From the Javadoc: https://github.com/Netflix/Hystrix/blob/master/hystrix-core/src/main/java/com/netflix/hystrix/strategy/executionhook/HystrixCommandExecutionHook.java#L141

* Invoked after completion of {@link HystrixCommand} execution that results in a response.
* 
* The response can come either from {@link HystrixCommand#run()} or {@link HystrixCommand#getFallback()}.

Is this not what you're seeing happen? If it is are you looking for a different behavior?

[mcacker]

I simplified the scenario, and removed the Fallback command. run() always fails, and getFallback() now returns just a new instance of the expected return type. I see onComplete called twice

onComplete via line 1107 (fallback)
onComplete via line 707 (execute)

I would have expected it just to be called once.

When onComplete is called, I fire an event, so I currently have code in place to ensure that it is only fired once, though I would prefer not to do that.

[benjchristensen]

Can you give me sample code, perhaps a unit test that reveals this behavior?