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

!!! FEATURE: Overhaul catch up implementation #4405

Merged
merged 16 commits into from
Aug 26, 2023

Conversation

bwaidelich
Copy link
Member

@bwaidelich bwaidelich commented Jul 14, 2023

Mainly adjusts the ProjectionInterface such that the iteration of events can happen outside of the implementation.

This allows to centralize logic which enables us to:

Breaking Change

This is a breaking change only if you implemented a custom implementation of the ProjectionInterface:

1. canHandle() now gets an instance of the domain EventInterface instead of the low level ES event

The code can be adjusted like this:

    public function canHandle(Event $event): bool
    {
        $eventClassName = $this->eventNormalizer->getEventClassName($event);
        // ...

=>

    public function canHandle(EventInterface $event): bool
    {
        $eventClassName = $event::class;
        // ...

2. catchUp() has been replaced with apply()

The code can be adjusted like this:

    public function catchUp(EventStreamInterface $eventStream, ContentRepository $contentRepository): void
    {
        // any catch up hook initialization here...
        $catchUp = CatchUp::create($this->apply(...), $this->checkpointStorage);
        $catchUp->run($eventStream);
    }

    private function apply(EventEnvelope $eventEnvelope): void
    {
        if (!$this->canHandle($eventEnvelope->event)) {
            return;
        }

        $event = $this->eventNormalizer->denormalize($eventEnvelope->event);
        // projection logic here...

=>

    public function apply(EventInterface $event): void
    {
        // projection logic here...

Note: The EventEnvelope gets passed in as 2nd argument and can be implemented if access to low-level details (e.g. event metadata) is needed:

    public function apply(EventInterface $event, EventEnvelope $eventEnvelope): void
    {
        // projection logic here...

3. getSequenceNumber() was replaced by getCheckpointStorage()

This enables the framework to use the CatchUp helper in the outer layers and it will allow us to implement better "blocking" mechanisms in the future

The code can be adjusted like this:

    public function getSequenceNumber(): SequenceNumber
    {
        return $this->checkpointStorage->getHighestAppliedSequenceNumber();
    }

=>

    public function getCheckpointStorage(): CheckpointStorageInterface
    {
        return $this->checkpointStorage;
    }

Resolves: #4289

@bwaidelich bwaidelich marked this pull request as ready for review August 4, 2023 14:58
@bwaidelich bwaidelich changed the title FEATURE: Overhaul catch up implementation (WIP) !!! FEATURE: Overhaul catch up implementation Aug 4, 2023
@skurfuerst
Copy link
Member

@bwaidelich IMHO this looks really good. Tests work still (as expected <3) So I'd say this is pretty much merge ready? (Not merging yet because the checkboxes ar enot yet ticked ;) )

use Neos\EventStore\Model\EventStream\EventStreamInterface;
use Neos\EventStore\Model\Event\SequenceNumber;

use function get_debug_type;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wasnt sure what we discussed, if we now like to adapt this code style or not - but it turns out we have yet no 100% clear decision: https://neos-project.slack.com/archives/C04Q0TC15/p1692352868964289?thread_ts=1683558218.323329&cid=C04Q0TC15

Copy link
Contributor

@dlubitz dlubitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. I just left some nitpic comments. 👍

@nezaniel
Copy link
Member

Otherwise, looks great! Let's merge

Copy link
Member

@nezaniel nezaniel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice, let's do this!

@bwaidelich bwaidelich merged commit bb8bc83 into 9.0 Aug 26, 2023
@bwaidelich bwaidelich deleted the feature/4289-overhaul-catchup branch August 26, 2023 12:29
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Feb 13, 2024
mhsdesign added a commit that referenced this pull request Feb 14, 2024
mhsdesign added a commit that referenced this pull request Feb 19, 2024
mhsdesign added a commit that referenced this pull request Feb 22, 2024
mhsdesign added a commit that referenced this pull request Feb 23, 2024
neos-bot pushed a commit to neos/contentrepositoryregistry that referenced this pull request Feb 26, 2024
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Mar 13, 2024
During extending phpstan to more packages (neos#4650) the migration was dropped.
The code had become already outdated by then as the api was changed a lot.
This service should function as boilerplate for new migrations.

WIP: Try to fix previous `EventMigrationService`

After neos#4405 a lot of things changed.

WIP: Write migration for workspace rename

untested.
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Mar 14, 2024
During extending phpstan to more packages (neos#4650) the migration was dropped.
The code had become already outdated by then as the api was changed a lot.
This service should function as boilerplate for new migrations.

WIP: Try to fix previous `EventMigrationService`

After neos#4405 a lot of things changed.

WIP: Write migration for workspace rename

untested.
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Mar 14, 2024
During extending phpstan to more packages (neos#4650) the migration was dropped.
The code had become already outdated by then as the api was changed a lot.
This service should function as boilerplate for new migrations.

WIP: Try to fix previous `EventMigrationService`

After neos#4405 a lot of things changed.

WIP: Write migration for workspace rename

untested.
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.

Overhaul CatchUp and CatchUpHook implementation
5 participants