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-1080]: Error handling #157

Merged
merged 56 commits into from
Jun 9, 2023

Conversation

cottinisimone
Copy link
Contributor

@cottinisimone cottinisimone commented May 26, 2023

https://prima-assicurazioni-spa.myjetbrains.com/youtrack/issue/PLATFORM-1080

I'm not totally sure this is an improvement. WDYT?

Pros:

  • Aggregate::Error now just contains Aggregate related errors only (command validation)
  • It is possible to simply understand between an Aggregate error and a Store error.

Cons:

  • AggregateManager type is not so smooth (now is something like AggregateManager<PgStore<AggregateA>>) while before it was with aggregate only AggregateManager<AggregateA>.
  • TransactionalEventHandler can return, in case you are using PgStore, a store error (sqlx or json) or a custom boxed error, not so easy to handle when returned being a generic std::error::Error.

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.

Minor comment on commented code

src/esrs/aggregate.rs Outdated Show resolved Hide resolved
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.

Minor comment on commented code

cpiemontese
cpiemontese previously approved these changes May 29, 2023
Base automatically changed from PLATFORM-1094/task/update-examples to master May 29, 2023 09:08
/// The store is protected by an [`Arc`] that allows it to be cloneable still having the same memory
/// reference.
#[derive(Clone)]
pub struct PgStore<A>

Choose a reason for hiding this comment

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

Why is the PgStore generic over the Aggregate?

If this was simply PgStore and then implement EventStore<A> we could have one singleton instance of the store that can be reused in the application. With the current version we need a specific store instance for each aggregate, which feels redundant (yes, I know the underlying PgPool is still shared, I'm talking about the convenience and developer experience of having multiple store instances around)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But event handlers and transactional event handlers sets are unique per aggregate..

Choose a reason for hiding this comment

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

Ah right... so as long as the store is in charge of the handlers it must be generic as well.

Bummers

@angelo-rendina-prima
Copy link
Contributor

Minor Qs as discussed in a huddle with @cottinisimone:

  • remove Clone from Aggregate::State
  • remove Send/Sync bounds from Aggregate associated types (if possible, those should be in the EventStore really)
  • for<'a> Aggregate::Event: 'a
  • try RwLock instead of ArcSwap

src/esrs/postgres/event_store.rs Outdated Show resolved Hide resolved
src/esrs/postgres/event_store.rs Outdated Show resolved Hide resolved
src/esrs/postgres/event_store.rs Outdated Show resolved Hide resolved
@cottinisimone cottinisimone requested a review from cpiemontese June 8, 2023 12:46
@cottinisimone cottinisimone merged commit 43a26c7 into master Jun 9, 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.

4 participants