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

Configurable Locking Scheme in SagaStore #947

Closed
smcvb opened this issue Jan 4, 2019 · 3 comments
Closed

Configurable Locking Scheme in SagaStore #947

smcvb opened this issue Jan 4, 2019 · 3 comments
Assignees
Labels
Priority 4: Would Lowest priority. Would-be-nice to include issues when time allows it. Status: Resolved Use to signal that work on this issue is done. Type: Feature Use to signal an issue is completely new to the project.
Milestone

Comments

@smcvb
Copy link
Member

smcvb commented Jan 4, 2019

It would be beneficial if the SagaStore could adjust it's locking scheme in regards to retrieving sagas from the database.

Ideally the Builder pattern would provide a toggle to change the query performed to retrieve a Saga instance. Doing so, a Saga instance could be enforce to not be acted upon concurrently by two distinct threads, for example when a Saga it's SagaManager is backed by a SubscribingEventProcessor.

Kick-off for this idea comes from Steven Grimm in this user group issue.

@smcvb smcvb added Type: Feature Use to signal an issue is completely new to the project. Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. labels Jan 4, 2019
@sgrimm-sg
Copy link

There was one thing I ran into in my 2.x implementation of this that went beyond using SELECT FOR UPDATE. The code needs to make sure it acquires locks in a particular order every time, or it risks deadlocks.

Simple example: host A publishes events of classes X and Y on its local event bus in that order, each of which is handled by a Saga on a SubscribingEventProcessor. At the same time, host B publishes events of classes Y and X. If both hosts naively acquire locks in the order the events arrive, host A will get the lock for the Saga that handles X, host B will get the one for the Saga that handles Y, and neither will be able to get the remaining lock.

This would apply even with locking schemes other than SQL row locks, of course.

In my 2.x implementation I solved it by looking at the Saga's UUID and doing a simple string comparison. If it was trying to load a Saga whose UUID was greater than any already-loaded ones, that was fine. If not, it would commit the UnitOfWork and start a fresh one with a new database transaction. In other words, it could only acquire locks in ascending Saga ID order within a single database transaction.

@abuijze abuijze added Priority 4: Would Lowest priority. Would-be-nice to include issues when time allows it. and removed Priority 2: Should High priority. Ideally, these issues are part of the release they’re assigned to. labels Aug 7, 2019
@smcvb smcvb added the Status: Under Discussion Use to signal that the issue in question is being discussed. label Sep 3, 2021
@smcvb smcvb added this to the Release 4.6.0 milestone Sep 3, 2021
@smcvb smcvb removed this from the Release 4.6.0 milestone Sep 1, 2022
@smcvb
Copy link
Member Author

smcvb commented Sep 1, 2022

Removed the milestone for now, as we will not be able to resolve this issue within release 4.6.0.
We will adjust this ticket accordingly once we pick it up again.

@smcvb smcvb added this to the Release 4.7.0 milestone Oct 3, 2022
@smcvb smcvb closed this as completed in d29a46b Jan 4, 2023
@smcvb
Copy link
Member Author

smcvb commented Jan 4, 2023

My initial hunch to support the described behavior from @sgrimm-sg by toggling it through Axon Frameworks (then still very new) Builder solution seemed unfitting after taking a look at the original changes just now.
As it stands, I could take the adjustments made in pull request #346 in it's entirety and place them in the PostgresSagaSqlSchema.
I did make some adjustments to the JavaDoc, but other than that, the intent is kept as is.

By doing so, I feel the discussion and envisioned solution in this thread is resolved sufficiently.
As such, I have closed the issue with commit d29a46b.

@smcvb smcvb self-assigned this Jan 4, 2023
@smcvb smcvb added Status: Resolved Use to signal that work on this issue is done. and removed Status: Under Discussion Use to signal that the issue in question is being discussed. labels Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority 4: Would Lowest priority. Would-be-nice to include issues when time allows it. Status: Resolved Use to signal that work on this issue is done. Type: Feature Use to signal an issue is completely new to the project.
Projects
None yet
Development

No branches or pull requests

3 participants