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

Core: state machine entity base class #1511

Merged
merged 7 commits into from
Jun 24, 2022

Conversation

algattik
Copy link
Contributor

@algattik algattik commented Jun 22, 2022

What this PR changes/adds

  • Extracted StateMachine utility class for state machine entities
  • Renamed previous StateMachine class to StateMachineManager for clarity and uniformity
  • Developer documentation on how to use/develop persistent state machine
  • Removed caching layer in in-memory store (as it added complexity with unclear benefits for code meant to be used for testing/at small scale)

Why it does that

  • Use the concept of state machine in a way that follows its definition
  • Reduce duplication in TransferProcess and ContractNegotiation and their in-memory stores
  • Make state machine easier to use together with state-machine-lib in extension projects.

Further notes

Also fixed race condition in ContractNegotiation (transition(DECLINED, DECLINING, PROVIDER_OFFERED, CONFIRMING, CONFIRMED, REQUESTED);), that manifested itself during a test run

Linked Issue(s)

Closes #1463

Checklist

  • added appropriate tests?
  • performed checkstyle check locally?
  • added/updated copyright headers?
  • documented public classes/methods?
  • added/updated relevant documentation?
  • added relevant details to the changelog? (skip with label no-changelog)
  • formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

@codecov-commenter
Copy link

codecov-commenter commented Jun 22, 2022

Codecov Report

Merging #1511 (955402f) into main (3584c73) will decrease coverage by 0.21%.
The diff coverage is 53.07%.

@@            Coverage Diff             @@
##             main    #1511      +/-   ##
==========================================
- Coverage   67.37%   67.16%   -0.22%     
==========================================
  Files         735      736       +1     
  Lines       16198    16139      -59     
  Branches     1058     1048      -10     
==========================================
- Hits        10913    10839      -74     
- Misses       4803     4823      +20     
+ Partials      482      477       -5     
Impacted Files Coverage Δ
...main/contract/negotiation/ContractNegotiation.java 0.00% <0.00%> (ø)
.../dataspaceconnector/spi/entity/StatefulEntity.java 0.00% <0.00%> (ø)
...iation/ConsumerContractNegotiationManagerImpl.java 86.95% <75.00%> (ø)
...iation/ProviderContractNegotiationManagerImpl.java 90.28% <75.00%> (ø)
...sfer/core/transfer/TransferProcessManagerImpl.java 81.54% <75.00%> (ø)
...ector/common/statemachine/StateMachineManager.java 94.11% <83.33%> (ø)
...tiationstore/InMemoryContractNegotiationStore.java 94.44% <90.90%> (+4.44%) ⬆️
...sferprocessstore/InMemoryTransferProcessStore.java 94.11% <92.30%> (-2.04%) ⬇️
...tor/core/defaults/InMemoryStatefulEntityStore.java 100.00% <100.00%> (ø)
...tor/spi/types/domain/transfer/TransferProcess.java 68.75% <100.00%> (-6.10%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3584c73...955402f. Read the comment docs.

@algattik algattik marked this pull request as ready for review June 22, 2022 04:38
@jimmarino jimmarino self-requested a review June 22, 2022 05:41
Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

What's the motivation behind modeling StatefulEntity as an interface and StateMachine as a superclass of TransferProcess and ContractNegotiation?
In my opinion they are not a StateMachine, they are StatefulEntity which state transitions are handled through a StateMachine, so the latter shouldn't be a superclass but something that interacts with the entities into the manager.

I would keep the StatefulEntity as an abstract class that would end up having an Entity superclass in turn being superclass of TransferProcess and ContractNegotiation.
The Entity class in the future would become the superclass of other non-stateful entities as Asset and PolicyDefinition

@algattik
Copy link
Contributor Author

algattik commented Jun 22, 2022

What's the motivation behind modeling StatefulEntity as an interface and StateMachine as a superclass of TransferProcess and ContractNegotiation? In my opinion they are not a StateMachine, they are StatefulEntity which state transitions are handled through a StateMachine, so the latter shouldn't be a superclass but something that interacts with the entities into the manager.

Formally, "A state machine is an abstract machine that can be in exactly one of a finite number of states at any given time" (https://en.wikipedia.org/wiki/Finite-state_machine) so the state machine is the entity, not the manager.

Nevertheless, I have renamed StateMachine to StatefulEntity, as this seems to make things clearer.

I would keep the StatefulEntity as an abstract class that would end up having an Entity superclass in turn being superclass of TransferProcess and ContractNegotiation. The Entity class in the future would become the superclass of other non-stateful entities as Asset and PolicyDefinition

As I understand this will be done by @diegogomez-zf as part of #1370. Is it ok to do things in two steps like this?

public B state(int value) {
entity.state = value;
return (B) this;
return self();
Copy link
Member

Choose a reason for hiding this comment

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

really clever method to avoid casting and checks suppression 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks - the only clever thing I did was to find a blog by someone cleverer :D

Copy link
Member

@paullatzelsperger paullatzelsperger left a comment

Choose a reason for hiding this comment

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

LGTM apart from minor nits.

We could look into introducing another base class Entity as superclass of StatefulEntity. Analogously we could devise an InMemoryEntityStore that in turn is a superclass of InMemoryEntityStore and takes those Entity objects.
There, we could store things like Policy, ContractDefnition, Asset, etc. That need not be done in this PR though.

docs/developer/state-machine.md Outdated Show resolved Hide resolved
docs/developer/state-machine.md Outdated Show resolved Hide resolved
@algattik
Copy link
Contributor Author

LGTM apart from minor nits.

We could look into introducing another base class Entity as superclass of StatefulEntity. Analogously we could devise an InMemoryEntityStore that in turn is a superclass of InMemoryEntityStore and takes those Entity objects. There, we could store things like Policy, ContractDefnition, Asset, etc. That need not be done in this PR though.

Yes indeed as I understand that is (at least in part) in scope for @diegogomez-zf in #1370.

@ndr-brt ndr-brt merged commit 385be86 into eclipse-edc:main Jun 24, 2022
juliapampus pushed a commit to FraunhoferISST/edc-connector that referenced this pull request Jun 24, 2022
* Core: state machine entity base class

* Update CHANGELOG.md

* Update EntitySendRetryManagerTest.java

* Update StateMachine.java

* Renamed StateMachine to StatefulEntity

* Update ContractNegotiationIntegrationTest.java

* grammar fix
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.

Core: base classes for entities and in-memory store
5 participants