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

Feature/613 lock entities #690

Closed

Conversation

ndr-brt
Copy link
Member

@ndr-brt ndr-brt commented Feb 14, 2022

Fix #613

Context

Currently, on the state machine loop, when an entity is processed asynchronously, it has to be put in an intermediate state to avoid the loop to fetch and process it again before the async operation completes.

What brings

The entities (TransferProcess and ContractNegotiation) can be locked.
A locked entity cannot be fetch by the nextForState query, it can only be fetch by id.
An entity gets unlocked on every state transition, so when a locked entity is fetched by id after the completion of an async operation and transitioned to another state, it gets unlocked automatically.
This will help the implementation of #416 , since an entity could be considered "stale" when it's locked for a certain amount of time.
There's also a refactoring, I didn't get why in the managers the stores were injected at start, now they are built into the managers through the builders.

@ndr-brt ndr-brt force-pushed the feature/613-lock-entities branch 4 times, most recently from c8641fa to 30b131a Compare February 14, 2022 15:33
@ndr-brt ndr-brt requested review from ronjaquensel, paullatzelsperger and jimmarino and removed request for ronjaquensel February 15, 2022 07:58
Comment on lines 340 to 350
var contractNegotiation = negotiationStore.find(negotiation.getId());
if (throwable == null) {
negotiation.transitionOffered();
negotiationStore.save(negotiation);
observable.invokeForEach(l -> l.providerOffered(negotiation));
contractNegotiation.transitionOffered();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think contractNegotiation should be checked for null here (like in the TransferProcessManagerImpl), otherwise might lead to a NullPointerException. Same for other methods in both *NegotiationManagers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you're right, I didn't do that because it was already done in #642, I didn't want to replicate too much logic here since I'll have enough conflicts to fix when it will be the time for rebase

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, I missed that it was done on the other branch.

@ndr-brt ndr-brt force-pushed the feature/613-lock-entities branch 10 times, most recently from 80a9241 to 0d9b2d1 Compare February 17, 2022 11:13
@ndr-brt
Copy link
Member Author

ndr-brt commented Feb 17, 2022

With @paullatzelsperger agreed that the lock mechanism should be handled with the lease on cosmos db, so that an entity is leased when it's fetched with nextForState and the lease will be released by the update operation.
In this way, it will simplier and the behavior will be like a SQL "select for update".
I will remove the locked flag logic from here and the lease thing will be implemented by paul in another PR

@ndr-brt ndr-brt force-pushed the feature/613-lock-entities branch 5 times, most recently from ffef89b to 5dd8bf2 Compare February 18, 2022 08:31
@ndr-brt
Copy link
Member Author

ndr-brt commented Feb 18, 2022

Replaced by #710

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.

[state machine review] Introduce locks on entities
2 participants