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

Unify EventHandler implementations. #461

Closed
wants to merge 1 commit into from

Conversation

Hc747
Copy link

@Hc747 Hc747 commented Oct 4, 2023

Motivation

The changes released in 4.0.0 are breaking compared to those available in 4.0.0.RC1 and mean that EventHandler's can either receive a sequence callback or have rewindable logic, but not both. These changes amalgamate the APIs so that an EventHandler can be both rewindable and able to receive sequence callbacks.

Changes

  • Remove EventHandlerBase component.
  • Remove RewindableEventHandler component.
  • Amalgamate and unify APIs of EventHandler, RewindableEventHandler and EventHandlerBase.
  • Revert RewindableException to RuntimeException instead of Throwable.
  • Denote BatchEventProcessor as being rewindable if internal RewindHandler is not an instance of NoRewindHandler.
  • Update DSL and builder API to account for above changes.
  • Update relevant test cases.

- Remove RewindableEventHandler component.
- Amalgamate and unify APIs of EventHandler, RewindableEventHandler and EventHandlerBase.
- Revert RewindableException to RuntimeException instead of Throwable.
- Denote BatchEventProcessor as being rewindable if internal RewindHandler is not an instance of NoRewindHandler.
- Update DSL and builder API to account for above changes.
- Update relevant test cases.
@Palmr
Copy link
Contributor

Palmr commented Oct 4, 2023

You are correct, "EventHandler's can either receive a sequence callback or have rewindable logic, but not both".
This is for good reason and I would question anyone wanting the ability to do both.

I'd suggest reading the PR that separated them #440 and importantly the associated Issue #437

Adding rewinding to the Batch Event Processor was a new feature for v4.
After the RC we realised it would be a very bad idea to have the ability to move the sequence on when the Batch Event Processor may rewind.

The fix for this that we implemented was to separate the implementations.
This should mean rewindability and getting a hook to the sequence have a separation enforced at compile time rather than runtime exceptions or the need to read documentation to find out why doing both would be a bad idea.

Since 4.0.0-RC1 was an RC, we were fine with adding in a breaking change atop that considering that it removed such an obvious footgun.

Especially as it was only a breaking change for anyone who'd jumped on the new-in-v4 feature of rewindable Batch Event Processor, the impact is minimal, and in a release full of breaking changes from 3.x anyway.

@Palmr Palmr closed this Oct 23, 2023
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

Successfully merging this pull request may close these issues.

2 participants