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: Event Publisher #256

Merged
merged 12 commits into from Jan 28, 2020
Merged

!!! FEATURE: Event Publisher #256

merged 12 commits into from Jan 28, 2020

Conversation

@bwaidelich
Copy link
Member

bwaidelich commented Jan 16, 2020

Introduces the notion of Event Publishers that are triggered whenever events are committed to the Event Store.
By default the provided JobQueueEventPublisher is used in conjunction with the DeferEventPublisher in order to retain the previous behavior (trigger catchup for all affected listeners in a new sub request at the end of the current request).

This makes the mechanism much more extensible and is a preparation for standalone use.

Breaking Change

This is a breaking change because there is no default Event Store configured out of the box any longer, so you need to adjust your Settings.yaml.
If you only used one Event Store the easiest fix is to re-configure the default Event Store:

Neos:
  EventSourcing:
    EventStore:
      stores:
        'default':
          storage: 'Neos\EventSourcing\EventStore\Storage\Doctrine\DoctrineEventStorage'
          listeners:
            '.*': true

But the new best practice is to define a (namespaced) Event Store for the main package / distribution that defines the events and listeners (see README).

Important: The Neos.EventSourcing.EventListener.listeners.* configuration that was introduced with #212 has been moved to Neos.EventSourcing.EventStore.store.*.listeners.
The old configuration is now ignored and you have to change:

Neos:
  EventSourcing:
    EventListener:
      listeners:
        'Some\Package\SomeCustomListener':
          eventStore: 'some_custom_store'
        'Some\Package\SomeOtherListener':
          eventStore: 'some_custom_store'

to:

Neos:
  EventSourcing:
    EventStore:
      stores:
        'some_custom_store':
          listeners:
            'Some\Package\Some(Custom|Other)Listener': true

Related: #180

bwaidelich added 2 commits Jan 16, 2020
tbd
bwaidelich added 3 commits Jan 17, 2020
..and adjust schema
@bwaidelich bwaidelich marked this pull request as ready for review Jan 17, 2020
Copy link
Member

skurfuerst left a comment

just some nitpick questions as basis for discussions

public function publish(DomainEvents $events): void
{
$queuedEventListenerClassNames = [];
foreach ($this->mappings->getMappingsForEvents($events) as $mapping) {

This comment has been minimized.

Copy link
@skurfuerst

skurfuerst Jan 20, 2020

Member

I don't yet understand this code :)

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jan 20, 2020

Author Member

Mh, true.. Too magic. It's basically a filter that returns only mappings that are relevant for one of the given $events.

Do you think a nested foreach would be easier to understand?

$queuedEventListenerClassNames = [];
$processedEventClassNames = [];
foreach ($events as $event) {
    $eventClassName = \get_class($event instanceof DecoratedEvent ? $event->getWrappedEvent() : $event);
    if (isset($processedEventClassNames[$eventClassName])) {
        continue;
    }
    $mappingsForEventClassName = $this->mappings->filter(...)
    foreach ($mappingsForEventClassName as $mapping) {
            $listenerClassName = $mapping->getListenerClassName();
            if (isset($queuedEventListenerClassNames[$listenerClassName])) {
                continue;
            }
            $queueName = $mapping->getOption('queueName', self::DEFAULT_QUEUE_NAME);
            $options = $mapping->getOption('queueOptions', []);
            $this->jobManager->queue($queueName, new CatchUpEventListenerJob($listenerClassName, $this->eventStoreIdentifier), $options);
            $queuedEventListenerClassNames[$listenerClassName] = true;
    }
}

or do you have a different idea?

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jan 22, 2020

Author Member

OK, I adjusted the code now with 19942a3 (and added a unit test to make sure that the end result is still the same).
It's still somewhat complex and maybe we can simplify this further in the future – but I would suggest to merge this one as is – unless you have a better idea on how to make that more readable of course!

bwaidelich added 4 commits Jan 21, 2020
by moving some of the filtering logic from the mappings
to the Event Publisher
@bwaidelich

This comment has been minimized.

Copy link
Member Author

bwaidelich commented Jan 22, 2020

@skurfuerst Thanks again for your feedback. I think I addressed all your concerns. Could you check once again?

@albe
albe approved these changes Jan 24, 2020
Copy link
Member

albe left a comment

Generally approve. Left some comments. I did like the "EventListenerTrigger" a more fitting name than "eventPublisher" - but, well, guess a "publisher" is a better known concept (though a tiny bit misleading in this case).

/**
* @return static
*/
public static function create(): self

This comment has been minimized.

Copy link
@albe

albe Jan 24, 2020

Member
Suggested change
public static function create(): self
public static function empty(): self

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jan 27, 2020

Author Member

You commented on an outdated class somehow, it's now EventToListenerMappings. I went for createEmpty() now to be in sync with DomainEvents::createEmpty()

This comment has been minimized.

Copy link
@albe

albe Jan 28, 2020

Member

Yeah... as said, I started twice with reviewing before :D

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jan 28, 2020

Author Member

I can imagine.. Not an easy one to review. Thanks for doing so anyways!

use Neos\Flow\Annotations as Flow;

/**
* An Event Publisher that sends events to a Job Queue using the Flowpack.JobQueue package.

This comment has been minimized.

Copy link
@albe

albe Jan 24, 2020

Member

It doesn't really send the events, but just a notification per listener that it needs to update. I think that's an important distinction and it would maybe even make sense to explain in short, why this is done (to not put the burden of deduplicating the events in the queue on the listener, so that it can just track the position and happily live his exactly-once-processing life)

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jan 27, 2020

Author Member

Good call, I tried to make this more clear: e39b269

BTW: More documentation is coming asap!

* @param string $projectionIdentifier
* @return bool
*/
public function isProjectionEmpty(string $projectionIdentifier): bool

This comment has been minimized.

Copy link
@albe

albe Jan 24, 2020

Member

Is this method not needed any more?

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jan 27, 2020

Author Member

No, it was just a left-over.
We might need something similar that allows listeners to specify some additional filters (See #141) but currently that part was not invoked anyways and IMO it should not matter whether a projection is "empty" – what does that mean anyways? :)

This comment has been minimized.

Copy link
@albe

albe Jan 28, 2020

Member

Well, empty means "no data/nothing projected" but fine with this. Was just concerned about breakiness

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jan 28, 2020

Author Member

Right – but IMO that should not be determined by the state of the projection but by the "highest applied sequence number". So the projector itself doesn't "know" about it necessarily

### Minimal Setup

With 2.0 there is no default Event Store configured any more.
Instead the initial Event Store setup is now expected to be defined in the corresponding package or distribution.

This comment has been minimized.

Copy link
@albe

albe Jan 24, 2020

Member

So, what does that mean for people upgrading? Is this "no default config" really necessary/beneficial? I prefer "zero config" setups, where all things basically work out of the box, but if I want, I can totally tune and reconfigure things to my liking.

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jan 27, 2020

Author Member

I had the same impression, but @skurfuerst and @kitsunet convinced me that we should strive for a "library" rather than a "framework" for this very package.
Reason: If you have a global Event Store, it's easy to get started but you run into problems as soon as you install another package that makes use of it.

Copy link
Member Author

bwaidelich left a comment

@albe thanks again for your feedback!
I tried to address your comments.

Regarding:

I did like the "EventListenerTrigger" a more fitting name than "eventPublisher"
but, well, guess a "publisher" is a better known concept (though a tiny bit misleading in this case).

In the framework we use the publisher only to trigger event listeners. But it could be used for all kinds of things on top of that (send out websocket events, flush caches, ...).

I took the name from Greg Youngs "Simple CQRS example"

@albe

This comment has been minimized.

Copy link
Member

albe commented Jan 28, 2020

No objections from my side

@bwaidelich bwaidelich merged commit 732538d into neos:master Jan 28, 2020
1 check passed
1 check passed
continuous-integration/styleci/pr The analysis has passed
Details
bwaidelich added a commit to neos/contentrepository-development-collection that referenced this pull request Jan 30, 2020
Mainly to the breaking changes introduced with neos/Neos.EventSourcing#256
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.