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

As requested: issue #13 #16

Closed
wants to merge 1 commit into from

Conversation

asiraky
Copy link

@asiraky asiraky commented Dec 20, 2015

#13 (comment)

The changes are not significant insofar as how the event gets pushed to the view models, but the concept of a 'Publish(event)' has been removed (including the implementation), so as not to create the false impression that event sourcing should follow the "store event -> publish event to queue" pattern. As stated by Greg, using an ESB creates all sorts of headaches when wanting to rebuild view models, or add new subscribers that require events from the beginning of time, not to mention the messaging issues created when not managing the ES & ESB within a distributed transaction.

~TLDR: The new subscriber semantics are designed to encourage the treatment of the event store as a commit log, doing away with any notion of an ESB, where any subscriber (new or old) can for whatever reason begin receiving events from the commit log (aka the event store) at any point in time (the position in the log), be it the start of time, or regular continuations of receiving the latest events

…he changes are not significant insofar as *how* the event gets pushed to the view models, but the concept of a 'Publish(event)' has been removed (including the implementation), so as not to create the false impression that event sourcing should follow the "store event -> publish event to queue" pattern. As stated by Greg Young, using an ESB creates all sorts of headaches when wanting to rebuild view models, or add new subscribers that require events from the beginning of time, not to mention the messaging issues created when not managing the ES & ESB within a distributed transaction.

~TLDR: The new subscriber semantics are designed to encourage the treatment of the event store as a commit log, doing away with any notion of an ESB, where any subscriber (new or old) can for whatever reason begin receiving events from the commit log (aka the event store) at any point in time (the position in the log), be it the start of time, or regular continuations of receiving the latest events
@asiraky asiraky changed the title As requested As requested: https://github.com/gregoryyoung/m-r/issues/13#issuecomment-165428366 Dec 20, 2015
@asiraky asiraky changed the title As requested: https://github.com/gregoryyoung/m-r/issues/13#issuecomment-165428366 As requested: issue #13 Dec 20, 2015
@MHacker9404
Copy link

Couple of questions about the code:
What is the purpose of the UnsubscribeToken?
If you can only have 1 commandHandler, wouldn't it make sense to make this explicit in the CommandBus? Instead of List<Action<Message>> handlers, would it be better to have Action<Message> handler in the CommandBus.RegisterHandler method?
Also what about an UnregisterHandler option?

Inquiring minds want to know :)

@gregoryyoung
Copy link
Owner

I'm on holidays and will try to review tomorrow.

@MHacker9404
Copy link

Here's another question:
If an Entity is constrained by it's invariants (thru the constructor), then it sounds like to me you don't want a public default constructor. I would envision 2 constructors - one based on the Created event, and one based on the events loaded from the repository. So in this example we wouldn't have a LoadFromHistory method - we would load the events and pass them to the constructor.

Is this valid?

@asiraky
Copy link
Author

asiraky commented Jan 8, 2016

The unsubscribe token is just there for detaching from the event stream at runtime. It's by no means necessary. I copied a portion of some code I use, which is why it made it in there.

  • Re the command handlers: yes, you should be constrained to just the one handler. The List is just a legacy from when the same bus was being used for publishing to multiple subscribers. The code will throw an exception if more than one handler is registered anyway, so given this is not production code it's probably not completely necessary that this be fixed with any real urgency,
  • Re the default ctor: the way the code is written is probably not exactly how you would do it in production, but the code is not meant for production. The code sample is geared towards demonstrating how easily a simple aggregate could be built from an event stream. I wouldn't focus too much on the implementation in the example.

The only reason I made the pull-request and changed the event stream was because at a conceptual level, it was incorrect and was leading people into thinking an esb was required, which is false. In fact, having the bus was very much antithetical to the architecture @gregoryyoung is trying to promote. I was in no-way critical of any of the implementation detail of the code, just that one core concept, which is rather important.

Again, my advice would be to not worry about the implementation detail too much. If you did, you would have to throw the whole example away as most - if not all of it - is not production worthy code. It's just meant as a tool to convey the concept of writing changes to an event stream via an aggregate, and using that event steam to build read models.

@asiraky asiraky closed this May 30, 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.

3 participants