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

[PLATFORM-573]: [esrs] POC different projectors on same table #99

Conversation

oliverbrowneprima
Copy link
Contributor

[PLATFORM-573]: First step - removed old examples, added the simpler new ones

@cottinisimone cottinisimone requested a review from a team August 29, 2022 10:46
Copy link
Contributor

@cottinisimone cottinisimone left a comment

Choose a reason for hiding this comment

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

I'm not sure about the simple_side_effect example. Maybe the naming of the examples too. We can discuss it a bit in a second moment!

examples/simple_projection/src/aggregate.rs Outdated Show resolved Hide resolved
// This is where some cleanup action would go in a more complex system
}
}
state + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not let the state be the counter of every successful logged messages? So that the state at this time could be changed here in the matches

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using the state here to illustrate the number of events persisted for a given command, so the person reading the example can easily see "ok so when I log a message an event is created to do the logging and then another is created due to the saga policy" etc. I don't mind moving it in to the match, but the idea was just to use it to illustrate the multiple events per command (obviously for the example the aggregate state isn't really necessary)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm also using it to illustrate how the state returned from handle_command is made invalid by the policy (see comments in main.rs), so always needs to be rebuilt from the DB - moving to inside the match arm for logging wouldn't allow me to do that easily

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but why don't give to the state more sense using it in the match..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's easier to illustrate to potential users about the invalid state if it's a simple count of persisted events, that's why.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not following the discussion 😆 what's the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Simone is asking why I'm using the AggregateState to track the number of events processed, rather than the number of lines logged, and I'm explaining that I'm using it to track the number of events persisted rather than the number of lines logged because it lets me demonstrate a couple of quirks of implementing a saga like this (using a Policy to send the Aggegate another command during the handling of the first one) slightly more straightforwardly.

It's not really a hugely meaningful difference, but I think this way allows me to be clearer in the comments describing what's going on. I'm not sure if/why a difference of opinion this minor is blocking

Copy link
Contributor

@cpiemontese cpiemontese left a comment

Choose a reason for hiding this comment

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

Some really minor comments.

Maybe we could include Alessandro and Francesco for more insightful comments than mine? 😆

examples/simple_projection/Cargo.toml Outdated Show resolved Hide resolved
examples/simple_projection/src/aggregate.rs Outdated Show resolved Hide resolved
examples/simple_saga/Readme.md Outdated Show resolved Hide resolved
@oliverbrowneprima
Copy link
Contributor Author

Some really minor comments.

Maybe we could include Alessandro and Francesco for more insightful comments than mine? laughing

Resolved and added, thank you!

@oliverbrowneprima oliverbrowneprima merged commit c7f6b0d into master Sep 1, 2022
@oliverbrowneprima oliverbrowneprima deleted the PLATFORM-573/user-story/esrs-poc-different-projectors-on-same-table-1 branch September 1, 2022 10:23
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