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

ISSUE-204 / Fix event publisher in aggregatestore/model #205

Merged
merged 1 commit into from
Mar 28, 2018

Conversation

maxekman
Copy link
Member

Summary

The events that have been published should be cleared in the aggregatestore/model EventPublisher.

How to test

Issue

Fixes #204.

Notes

@maxekman
Copy link
Member Author

@superchalupa I think this will fix your issue.

@maxekman maxekman merged commit 7067a22 into looplab:master Mar 28, 2018
@maxekman maxekman deleted the ISSUE-204/fix-eventpublisher branch March 28, 2018 08:20
@@ -75,7 +75,9 @@ func (r *AggregateStore) Save(ctx context.Context, aggregate eh.Aggregate) error

// Publish events if supported by the aggregate.
if publisher, ok := aggregate.(EventPublisher); ok && r.bus != nil {
for _, e := range publisher.EventsToPublish() {
events := publisher.EventsToPublish()
publisher.ClearEvents()
Copy link

Choose a reason for hiding this comment

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

Will we lost all of events if an error happen?

Copy link

Choose a reason for hiding this comment

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

This line should move to after the loop

Copy link
Member Author

Choose a reason for hiding this comment

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

I choose the same "guarantee" as the ES version, which is to clear all events before publishing, no matter what the result of publishing is. If an error happens here it will trickle to the command handler anyway, and should be handled by the client with a new request. There is a much broader issue about what to do with failing events in #181.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it will work. I'll apply and check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Everything checks out. The old code crashed without my hack. Everything works ok with upstream master now. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants