The Wayback Machine - https://web.archive.org/web/20201117185257/https://github.com/eventflow/EventFlow/pull/764
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

Ms SQL shoud throw OptimisticConcurrencyException on read model insert conflict #764

Open
wants to merge 1 commit into
base: develop
from

Conversation

@leotsarev
Copy link
Contributor

@leotsarev leotsarev commented Jun 17, 2020

If we got unique constraint violation on inserting read model it probably another thread already inserted read model and we need to start update again.

@leotsarev leotsarev force-pushed the FortisOnline:throw-optimistic branch from 3ff0910 to 4483b2f Jun 17, 2020
@leotsarev
Copy link
Contributor Author

@leotsarev leotsarev commented Jun 17, 2020

Another option could be to implement retry directly in MsSQLReadStore, but we will lose ability to use RetryPolicies (or will really-really complicate MsSQLReadStore)

@rasmus rasmus added the enhancement label Jun 18, 2020
@rasmus
Copy link
Member

@rasmus rasmus commented Jun 18, 2020

If you have a 1-to-1 relationship between your read model and your aggregate, you could try the AggregateReadStoreManager which checks if the version is as expected, applies any missing events and the updates the read model.

@leotsarev
Copy link
Contributor Author

@leotsarev leotsarev commented Jun 18, 2020

Thanks, that's interesting. It's +1 to this implementation (not retrying inside MsSQLReadStore).
However for that to work we need read store to signal that concrete Sql exception is probably OptimisticConcurrencyException, because various generic ReadStoreManager do not know that concrete code of SqlException XXX could be retried specifically in case of INSERT.
So, for me that change is net win.
If you have time, could you please reword your comment to be more actionable (leotsarev, please investigate this or that, try another approach, also add fix here or there, request review from somebody, add tests...)

@rasmus
Copy link
Member

@rasmus rasmus commented Aug 20, 2020

@frankebersoll is this still relevant after you merged #768?

@leotsarev
Copy link
Contributor Author

@leotsarev leotsarev commented Aug 20, 2020

I think it's still relevant.
Before change + SingleAggregateReadStoreManager
If two threads (A & B) will try to create read model in parallel, one of them could fail and lose change.
Before change + AggregateReadStoreManager
If two threads (A&B) will try to create read model in parallel, one of them could fail and lose change (say, B).
Sometimes later when another update (C) to this aggregate happen, update B will be added to readmodel in correct order
After change
If two threads (A & B) will try to create read model in parallel, one of them could fail and signal OptimisticConcurrencyException. It will be retried.

@leotsarev
Copy link
Contributor Author

@leotsarev leotsarev commented Oct 28, 2020

@rasmus
Copy link
Member

@rasmus rasmus commented Oct 28, 2020

Still relevant, haven't had the time to give it a go yet though

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.