General feedback

[benjamindulau]

Hi,

Here is my general feedback, for what it worths, on this very early version of the code.

First of all, I think that there is too much useless complexity. The use of traits is nice for separation of concerns but can be very confusing. These are internals and I would rather take a mixed approach between your approach and the one from @beberlei, meaning a balance between purity and pragmatism.

I would have a more simple AggregateRoot base class:

<?php

abstract class AggregateRoot
{
    /**
     * @var DomainEvent[]
     */
    private $recordedEvents = [];

    /**
     * Reconstitute the AggregateRoot state from its event history
     */
    protected function reconstituteFromHistory(DomainEventCollection $history)
    {
        foreach($history as $event) {
            $this->apply($event);
        }
    }

    /**
     * Ask execution of the given Domain Event behavior
     * and add the event to the recorded events to be committed.
     */
    protected function recordThat(DomainEvent $event)
    {
        $this->recordedEvents[] = $event;
        $this->apply($event);
    }

    /**
     * Tries to call the apply* behavioral method corresponding
     * the given event.
     */
    private function apply(DomainEvent $event)
    {
        $method = 'apply' . EventNameGuesser::guess($event);
        if(is_callable([$this, $method])) {
            $this->{$method}($event);
        }
    }

    /**
     * Clears the recordedEvents and returns them
     */
    public function popRecordedEvents()
    {
        $events = new DomainEventCollection($this->recordedEvents);
        $this->recordedEvents = array();

        return $events;
    }
}

Simple enough and does the job, but that's my opinion. We don't need so much abstraction here.

Also the "Contract" thing adds nothing for me. Having a simple utility class extracting the Stream name directly from an Aggregate or an Event would be enough. No need for that complexity. I understand that it adds flexibility but I prefer simplicity ;-)

Now regarding the EventStore, I find it weird for the EventStream to get a persistence dependency directly and to commit "itself" to the persistence. But I think you're already doing something regarding that matter.

I would personally represent explicitly the commit transaction with a Commit object holding the commitId and the committed events. @beberlei did something similar with his "Transaction" object. I find it useful, especially if you want to do something with the committed events after they are actually committed ;-)

And then I'd have something like the following:

<?php

class WhateverEventStore
{
    public function commit(EventStream $stream)
    {
        $commit = $stream->createCommit();
        // internally does something like:
        // return new Commit(CommitId::generate(), $this->streamId, $this->getPendingEvents());

        $this->persistence->add($commit);
        $stream->clear(); // clears the pending events & merge them in committed events, don't like the method name though, need to think more about it.

        return $commit;
    }
}

What I like however is the UnitOfWork. I don't like that the handler must actually call the $uow->commit(); itself though, but I don't have a solution for that right now ;-)

My other remarks are about vocabulary and are not so relevant. For instance I don't like the name "EventEnvelope", I would simply call it "StreamEvent" since it represents an event inside the stream itself .

That's it for now.

Nice work!

[mathiasverraes]

Thanks for the feedback! I'll leave this issue open for now, to remind myself to reconsider your ideas later.

Simplicity is important. But there are other goals that may be less obvious. EventCentric aims to solve some of the problems I had with evolving CQRS/ES systems in production. Events change, aggregates change, event stores need to be partitioned, bugs cause incorrect events to be recorded, auditing, different programming languages... I'm trying to make the library I wish I had back then. I don't have all the answers, but I intend to keep redesigning until I have :-)

The contracts are an attempt to make it explicit that events have a public contract that defines them and that is documented somewhere. In a php-only system, using FQCNs is fine, but in more complex, you may have mixed naming schemes, etc. The streams need contracts as well, because ids might be recycled across aggregates. In fact I had this quite often.

I want the end user to be able to describe a domain model in nothing but domain language. For example, the term "aggregate id" is not part of the language, so it doesn't exist in the model. Only the implementation of the repository knows which domain id to pick from the aggregate to use as aggregate id. I'd rather have simplicity in the domain model, even if that comes at a cost in the infrastructure.

Keep the suggestions coming. In fact, would you be interested in pairing with me for a couple of days? Progress has been slow but I'm hoping to have some more spare time in the coming weeks.

[benjamindulau]

I'm currently very busy on projects, but like you, in the coming weeks I should be able to find some time. But that would be current september at minimum :/

Also note that I don't have the vision you have. Mine is really project-to-project and I don't have so much experience using an event store. I'm just currently trying to implement a very custom one for a project revamp I work on and in which I'd like to try a wide CQRS + ES approach. That's why I'm very cautious about my feedback until I'm sure I get every concept right ;-)

But maybe you could benefit from some of my ideas.

Let's keep in touch about that, and in the mean time I'll try to organize my ideas and keep adding comments.

[mathiasverraes]

I find that pairing with people forces me to get out of my head and look at things differently. Mail me when you have some dates.

On 18 August 2014 21:32, Benjamin Dulau notifications@github.com wrote:

I'm currently very busy on projects, but like you, in the coming weeks I should be able to find some time. But that would be current september at minimum :/

Also note that I don't have the vision you have. Mine is really project-to-project and I don't have so much experience using an event store. I'm just currently trying to implement a very custom one for a project revamp I work on and in which I'd like to try a wide CQRS + ES approach. That's why I'm very cautious about my feedback until I'm sure I get every concept right ;-)

But maybe you could benefit from some of my ideas.

Let's keep in touch about that, and in the mean time I'll try to organize my ideas and keep adding comments.

— Reply to this email directly or view it on GitHub https://github.com/event-centric/EventCentric.Core/issues/16#issuecomment-52542762 .

Mathias Verraes mathias@verraes.net http://verraes.net @mathiasverraes http://twitter.com/mathiasverraes

[benjamindulau]

Just an idea, but why don't having an UOW working on the EventStream(s) directly instead of the Aggregates?

In your UOW, something is weird. The fact that you call createStream in the persistAggregate, the last being called whatever it is a new Aggregate or a loaded one. In the case where the Aggregate was already loaded before (get method) doing any operation, its EventStream should already exist in memory. That's why I was thinking about having a UOW working directly on the streams.

Plus, having a tracked EventStream would enforce the "commit" meaning.

Note that I just had this idea and I'm still searching for flaws in it... Just writing as it comes.

[benjamindulau]

I'm having a look at your V2 stuff. I still don't understand why you need so much unique information.

For me, for the EventStream, the StreamId (potentially defined by the user) is enough. It should be either given by the user or generated automatically, but one ID is strong enough, I don't get the need of the StreamContract. Could you give me your reasoning about that?

The same goes for the Event, we only need an event type (user defined), and an internally generated EventId.

Globally, IMO, a Commit should be something like the following:

• StreamId
• CommitId (internally generated)
• CorrelationID (can be user-defined)
• CausationID (can be user-defined)
• CurrentVersion (user-defined, for optimistic concurrency check)
• EventList, an Event (in the stream, not the DomainEvent) being:
  • StreamId
  • EventType (user-defined)
  • EventId (internally generated)
  • EventDate or EventNumber (for maintaining the sequential nature of the events)
  • EventPayload
  • Metadata

Would you mind giving me some insight about your reasoning on partitioning and buckets too?

[benjamindulau]

Hey,

Here are my new thoughts about all this. I've been thinking a lot about it in spite of myself, stupid brain, won't you shut up? ;-)

That's what I would do:

First, I would separate concepts for the Event Store from the concepts of the domain (DomainEvent / Aggregates). But that's what you did, or at least partly.

An EventStore is no more than event streams in which we append events. This is not related directly to the domain. So I'd separate the two concepts. I also would make some stuff more explicit, meaning more in the language of the event sourcing "domain" (ubiquitous langage).

First, I would get rid of the StreamContract and keep only the StreamId being a string identifier, possibly user-defined or automatically generated otherwise.

Next, I would rename EventEnvelope to something like StreamEvent because it has more meaning in the event sourcing language. I would also use a dedicated Value Object for the EventContract instead of using directly a Contract instance, which is the same class being used in all the layers by the way, that's confusing. I would call it something like EventType and it would maybe extend the Contract class, just like you did for the identifiers. Again, more meaning in the ES language.

Note that now I understand the use of the contract thing, was not obvious before.

As for the EventStore, I would make it really simple and agnostic from the rest of the library. It would be an interface like:

<?php

interface EventStore
{
    /**
     * @param StreamId          $streamId
     * @param AppendTransaction $transaction
     * @return Commit
     */
    public function appendToStream(StreamId $streamId, AppendTransaction $transaction);

    /**
     * @param StreamId $streamId
     * @param string   $version
     * @return EventStream
     */
    public function getStream(StreamId $streamId, $version = null);
}

An AppendTransaction being something like:

<?php

class AppendTransaction
{
    private $correlationId;
    private $causationId;
    private $expectedVersion;
    private $events;

    /**
     * @param CorrelationId $correlationId
     * @param CausationId   $causationId
     * @param int           $expectedVersion
     * @param StreamEvent[] $events
     */
    public function __construct(CorrelationId $correlationId, CausationId $causationId, $expectedVersion, array $events)
    {
        // TODO: assert that $events contains StreamEvent instances only
        Assert\that($expectedVersion)->integer()->min(0);
        $this->correlationId = $correlationId;
        $this->causationId = $causationId;
        $this->events = $events;
    }

    /**
     * @return CausationId
     */
    public function getCausationId()
    {
        return $this->causationId;
    }

    /**
     * @return CorrelationId
     */
    public function getCorrelationId()
    {
        return $this->correlationId;
    }

    /**
     * @return int
     */
    public function getExpectedVersion()
    {
        return $this->expectedVersion;
    }

    /**
     * @return StreamEvent[]
     */
    public function getEvents()
    {
        return $this->events;
    }
}

(like the geteventstore from greg young does).

appendToStream would immediately create a commit and persist it in the EventStream. Not sure about the getStream method though. I'm not sure we should manipulate directly an EventStream at all.

And that's it, the event store is no more than that.

Everything else is some sort of a bridge between the domain stuff and the event store and should live in a different namespace.

I'd would keep the UnitOfWork which is really nice (forget about my previous comment on having a UOW on the EventStream, doesn't make sense at all).

The persistAggregate method would do something like that:

<?php

$domainEvents = $aggregate->getChanges();

$transaction = new AppendTransaction(
    CorrelationId::generate(),
    CausationId::generate(),
    1, // expected version, must think more about that
    $this->streamEventFactory->fromDomainEvents($domainEvents)
);

$commit = $this->eventStore->appendToStream(
    StreamIdGenerator::fromAggregate($aggregate->getAggregateRoot(), $aggregate->getAggregateId()),
    $transaction
);

$aggregate->clearChanges();

return $commit;

?>

It then would be the user job to wrap this into some sort of a EventStoreRepository class, which internally use the UOW and make something after a commit() operation like emitting all the events from the returned Commit instance for projection or whatever.

I think that's it. Hope it makes sense to you ;-)