Rewindable event handler separation

[Palmr]

This should give us type-safe rewindable and non-rewindable EventHandler implementations.

Non-rewindable implementations will not be able to throw RewindableException but will be able to implement EventHandler::setSequenceCallback and the opposite for rewindable implementations.

This will fix #437

[swarren12]

Rather than a boolean, could we have behaviour?

[swarren12]

Thinking something like adding an private inner interface:

private final interface RewindBehaviour
{
      long onRewindableException(RewindableException e);
}

and then there could be two private methods:

private long beKindRewind(final RewindableException e)
{
    if (this.batchRewindStrategy.handleRewindException(e, ++retriesAttempted) == REWIND)
    {
        return startOfBatchSequence;
    }
    else
    {
        retriesAttempted = 0;
        throw e;
    }
}

private long rewindNotAllowed(final RewindableException e)
{
    throw new RuntimeException("Rewindable Exception thrown from a non-rewindable event handler", e);
}

Constructors would then take this::beKindRewind in place of true and this::rewindNotAllowed in place of false? (one catch here is that they might not be referencable in this() calls, so eh?).

This would save continuously checking an if that will never change; on the other hand it does possibly make navigation slightly harder?

Also, this really only works because retriesAttempted appears to be instance scoped? I'm assuming this is so we persist failures across calls until we get a success, but it's not entirely clear to me?

[grumpyjames]

How about 'The created BEP will not support batch rewind". The constructor is just a constructor

[grumpyjames]

Same same - the constructor isn't supporting anything!

[grumpyjames]

Gentle preference for EventHandlerBase over BaseEventHandler, which sounds like an EventHandler that someone has done something unpleasant to.

[grumpyjames]

I sort of hate that we've got three constructors on BEP; if it weren't such a ballache I'd be suggesting some named factory methods instead.

I agree with Simon's suggestion of behaviour rather than boolean.

Don't think any of my comments are showstoppers.

[sea36]

RewindableException extends Throwable

Extending Throwable :(
https://stackoverflow.com/a/2756001

And if we're going that path, it isn't an Exception any more.

[swarren12]

RewindableException extends Throwable

Extending Throwable :( https://stackoverflow.com/a/2756001

And if we're going that path, it isn't an Exception any more.

I also questioned this offline - my understanding of the reasoning here is that we have no way of allowing the regular handlers to throw an arbitrary exception but not a RewindableException. Making it an Error wouldn't work, as these are unchecked, so the only other option would be to change the signature of onEvent() in EventHandler.

Something like this would work in theory:

void onEvent(T event, long sequence, boolean endOfBatch) throws DisruptorException;

but that would require catching and wrapping every Exception, and would probably make migration onto 4.0 painful.

[Palmr]

On the topic of exceptions and use of Throwable: I think it's justified here as it's a specific exception we want to raise in a certain circumstance.

There's probably some major refactoring I'd like to make in a future major release to make EventHandler::onEvent return something as right now we're straying down the path of exceptions for control flow. Another case in #397

For now I think it's a fair trade off as the RewindableException is not something that should bubble out past the BatchEventProcessor and concerns of it not being caught by those only catching Exception should be mostly moot in this case.

[swarren12]

LGTM

Wonder if we can move the retriesAttempted variable off the handler into the retry strategy as a next-step refactor?

[grumpyjames]

Man this interface is appalling for reviewing.