[PLATFORM-1073]: Remove projectors and policies in favour of event handlers #148
Conversation
cottinisimone
requested review from
matteosister,
visd0m,
mbusi,
MatteoJoliveau and
LeartS
cpiemontese
changed the title
| // 2. the event handlers below might need to access this aggregate atomically (causing a deadlock!). | ||
| drop(aggregate_state.take_lock()); | ||
|
|
There was a problem hiding this comment.
Have we checked there are no implications around locking, with the refactor?
I don't see anything obvious but worth double checking
There was a problem hiding this comment.
You are right. Maybe it's worth to explicit write that in the transactional event handlers is mandatory not to call other aggregates and that are intended to be used for the read side only
Yes, they'll be fixed in further PRs. The work we are doing is larger than this. Sorry for the big changes :(. I know is tedious.. |
| /// 2. load | ||
| /// 3. lock_and_load | ||
| /// The other functions are used internally, but can be overridden if needed. | ||
| #[async_trait] | ||
| pub trait AggregateManager: Aggregate { |
There was a problem hiding this comment.
What about transforming this AggregateManager in something similar to the command bus of the proposal of @MatteoJoliveau, allowing to publish the produced events on the event bus after the storing of the events.
The handle_command could become something like:
- load aggregate
- handle command on the aggregate
- store events
- publish the produced events on the event bus (whatever we mean with event bus 😅)
I was thinking at this to give the idea.
There was a problem hiding this comment.
Or maybe this will be covered in a following pr 🤔
There was a problem hiding this comment.
Ehi @visd0m we are doing those kind of stuff in different PRs. This is the PR related to the aggregate manager (and bounds in codebase) refactor. Wdyt? Can we discuss this changes in the other PR maybe?
There was a problem hiding this comment.
Sure, sorry I didn't notice the other pr 🙏.
There was a problem hiding this comment.
Looking forward to see other prs more focused on taking some design decision from @MatteoJoliveau proposal.
| } | ||
| } | ||
|
|
||
| // FIXME: uncomment |
There was a problem hiding this comment.
You are right. This will come in further PR. We was thinking about a final PR to embellish the codebase with some ribbons
| // TODO: doc | ||
| async fn handle(&self, event: &StoreEvent<M::Event>); | ||
|
|
||
| // TODO: doc | ||
| async fn delete(&self, _aggregate_id: Uuid) {} |
| } | ||
| } | ||
|
|
||
| pub trait ReplayableEventHandler<M: AggregateManager>: EventHandler<M> + Send + Sync {} |
There was a problem hiding this comment.
What's this used for?
At the very least let's have a docstring explaining what this is! I only see it used in an example
There was a problem hiding this comment.
The idea is mostly to use in the eventually-consistent side. This marks event handlers to be replayable, that means that you can safely replay it in case of an error, or you can re-run it while rebuilding
| AM: AggregateManager, | ||
| { | ||
| // TODO: doc | ||
| async fn handle(&self, event: &StoreEvent<AM::Event>, executor: &mut E) -> Result<(), AM::Error>; |
There was a problem hiding this comment.
Why does this take an executor explicitly, whereas EventHandler does not?
I assume it's to actually pass the current db transaction through?
There was a problem hiding this comment.
Yes, it's for that. TransactionalEventHandlers have been created in order to have async-transactional event handlers as projector were before this change
| #[async_trait] | ||
| pub trait EventHandler<M: AggregateManager>: Send + Sync { | ||
| // TODO: doc | ||
| async fn handle(&self, event: &StoreEvent<M::Event>); |
There was a problem hiding this comment.
Unlike TransactionalEventHandler, this is infallible and does not take an executor to avoid passing an active transaction and we explicitly ignore nested errors, right?
There was a problem hiding this comment.
@cottinisimone re: errors, are you using the same strategy as my PoC?
In that case, I would say "we handle errors locally in the handle method", rather than "ignore". Just a semantic difference
|
@angelo-rendina-prima is it ok if we address some of these comments in the next PRs? do you think we can merge this PR and move forward with the next round? |
cottinisimone
deleted the
PLATFORM-1073/task/streamline-projectors-and-policies
branch
https://prima-assicurazioni-spa.myjetbrains.com/youtrack/issue/PLATFORM-1073
This PR removes projectors and policies in favor of generic event handlers, which can be transactional or not