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

Stream type #181

Closed
wants to merge 12 commits into from
Closed

Stream type #181

wants to merge 12 commits into from

Conversation

wjzijderveld
Copy link
Member

This is a start to add the stream type (in most cases the Aggregate Root) to the EventStore.

Still TODO:

  • Check if we need to normalize the classname
  • Update CHANGELOG
  • Give some hints/snippets to add a migration to your application
  • Provide a more sensible name, I don't really like stream type here

@wjzijderveld
Copy link
Member Author

This issues relates to #111 and #120

@simensen
Copy link
Contributor

Is there anything I can do to help this along?

@wjzijderveld
Copy link
Member Author

Not sure how you can help right now, I would like to have some more people involved to discuss this in more detail.. I'm just not sure this is the solution, so I'm curious to other opinions and ideas.

@jacobkiers
Copy link
Contributor

On the plus side, this implementation brings Broadway closer to the implementation of a few other ES implementations, as described in #120. That in itself would make it useful, I think.

On the other hand, this breaks BC. I know we're not yet at 1.0, so technically that would not be a problem. In my experience however, breaking BC is always painful, especially more so when it impacts data stores 😬. So if we do go this route, I would like a more BC-friendly solution.

Personally, for us at Alphacomm, we just use the DBAL Event store with different tables for each aggregate. That solves the problem nicely, and the problem as described in #120 immediately goes away as well...

All in all, I'm moderately positive about the idea.

With regards to naming: why not just name it something like $aggregateType? Since AFAICS the Event Store is exclusively used to store events for aggregates, making that explicit is a good thing, I think.

@wjzijderveld
Copy link
Member Author

I'm not sure about aggregateType, personally I'm fine with it, but as aggregate is a term related to DDD and not event sourcing per se, I'm not sure it's the best fit.

@jacobkiers And I'm sorry, but BC breaks will happen... We deliberatly didn't tag a 1.0 yet for that specific reason, as we are aware that there will be more BC breaks coming.

We will tag 1.0 eventually, but I would like to have more people using it to find some issues that might still exist, especially in the newer stuff like Snapshotting and the event store management stuff.

@@ -23,11 +23,12 @@
*
* @return DomainEventStreamInterface
*/
public function load($id);
public function load($streamType, $identifier);
Copy link
Member

Choose a reason for hiding this comment

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

$streamType 👍 or just $type (you already know you're getting a DomainEventStreamInterface

@asm89
Copy link
Member

asm89 commented Jul 25, 2015

@wjzijderveld 👍 for the work on this, it had to happen eventually. :) The sooner we merge this, the better imo.

Check if we need to normalize the classname

I suggest to do the same as we currently do for events?

Give some hints/snippets to add a migration to your application

Yes, that'd be nice. It really depends on how you structure your own code though. I'm still thinking about this one.

Provide a more sensible name, I don't really like stream type here

See inline comment (+1 on stream type, or just type, since we're dealing with streams already).

@jacobkiers A table per aggregate root? That's an interesting approach. These additions should perfectly fit your use case?

@andersonamuller
Copy link

GetEventStore calls it category.

Stream names in the Event Store are strings, and we have the convention which can be used whereby stream names of the format category-rest use the prefix before the hyphen as the event category. This can be useful later when using projections (Greg will be covering using them in his blog series on projections).

Because the category name is used from JavaScript, the default implementation of aggregateIdToStreamName converts the name to camel case - so an aggregate of type MyTestAggregate with an ID of ffe312b9-624a-4a2a-9665-e9ae27dd1d7d ends up with the stream name myTestAggregate-ffe312b9-624a-4a2a-9665-e9ae27dd1d7d.

https://geteventstore.com/blog/20130220/getting-started-part-2-implementing-the-commondomain-repository-interface/

@renan
Copy link

renan commented Jul 31, 2015

Couldn't it be a implementation detail? I imagine a DBALEventStore taking streamType in the constructor. Making one Event Store per Aggregate Root, like Repositories are.

@francescotrucchia
Copy link
Contributor

👍

1 similar comment
@Miliooo
Copy link
Contributor

Miliooo commented Sep 15, 2015

👍

@Xerkus
Copy link
Contributor

Xerkus commented Sep 15, 2015

👎 to your 👍

@NoelDavies
Copy link
Contributor

Has any progress been made to snapshotting? Would be interested in taking a look! We use Broadway for a very specific sector in London and it works great, but going forward we can see snapshots becoming very useful in handling the number of events we're expecting.

@othillo
Copy link
Member

othillo commented Nov 29, 2016

@NoelDavies I was looking into snapshotting myself last week. I started with a basic snapshotting repository and a snapshotting aware event store. I will submit a POC.

@mhightower
Copy link

mhightower commented Dec 21, 2016

I'm not too familiar with GitHub but is the branch below with the passing checks the POC @othillo was talking about?

@othillo
Copy link
Member

othillo commented Feb 25, 2017

@mhightower you can find my POC at https://github.com/othillo/broadway-snapshotting

@othillo
Copy link
Member

othillo commented Feb 25, 2017

@wjzijderveld what is the status of this PR?

@wjzijderveld
Copy link
Member Author

We eventually decided not to continue with this, but decided to configure different event stores. So we never finished this integration.

I'll close this for now, if somebody wants to continue on this we can reopen, but it is already so old, that simply opening a new PR might be easier.

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.