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

LockedLoad #194

Merged
merged 4 commits into from
May 8, 2024
Merged

LockedLoad #194

merged 4 commits into from
May 8, 2024

Conversation

angelo-rendina-prima
Copy link
Contributor

No description provided.

/// It's essentially `Option<AggregateState<T>>`, but it also needs to
/// retain the lock information.
enum LockedLoadInner<T> {
None { id: Uuid, lock: EventStoreLockGuard },
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do this need to have an id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the use case is:

  • you do lock_and_load(ID)
  • you get Empty { ID, ... }
  • you do .unwrap_or_default which gives you AggregateState<T>::default().with_id(ID).with_lock(lock)

TLDR: LockedLoad is the result of trying lock_and_load on a specific ID.

Copy link
Contributor Author

@angelo-rendina-prima angelo-rendina-prima May 3, 2024

Choose a reason for hiding this comment

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

I guess there can be a scenario where you do

result = lock_and_load(123)
if result is None
  result = AggregateState::new() // <-- completely new ID, say 456

so we likely want LockedLoad::is_none, LockedLoad::is_some, LockedLoad::unwrap_or, LockedLoad::unwrap_or_else utilites to support this use case!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, makes sense

/// It's essentially `Option<AggregateState<T>>`, but it also needs to
/// retain the lock information.
enum LockedLoadInner<T> {
None { id: Uuid, lock: EventStoreLockGuard },
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, makes sense

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.

Only small suggestion about constructors visibility. I don't think user should be able to create a new instance of this struct

@angelo-rendina-prima angelo-rendina-prima force-pushed the fix-no-lock-on-empty-load branch from 3a6c96e to 5595129 Compare May 7, 2024 13:09
@angelo-rendina-prima angelo-rendina-prima force-pushed the fix-no-lock-on-empty-load branch from 5595129 to 8ab3e7b Compare May 7, 2024 13:10
cottinisimone
cottinisimone previously approved these changes May 7, 2024
@angelo-rendina-prima angelo-rendina-prima marked this pull request as ready for review May 8, 2024 07:44
@angelo-rendina-prima angelo-rendina-prima requested a review from a team as a code owner May 8, 2024 07:44
@cottinisimone cottinisimone merged commit 64f80a9 into master May 8, 2024
7 checks passed
@cottinisimone cottinisimone deleted the fix-no-lock-on-empty-load branch May 8, 2024 07:46
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.

2 participants