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: apply 2-state transition pattern to TransferProcessManager #831

Merged
merged 1 commit into from
Mar 16, 2022
Merged

core: apply 2-state transition pattern to TransferProcessManager #831

merged 1 commit into from
Mar 16, 2022

Conversation

ndr-brt
Copy link
Member

@ndr-brt ndr-brt commented Mar 7, 2022

What this PR changes/adds

Migrate all the state transition to the 2-state pattern as described in #825 for TransferProcessManager
(PRs for ContractNegotiationManagers are about to come)

Why it does that

Use the 3-state pattern could lead to problems, described in #825

Further notes

now all the states (except ERROR, CANCELLED, ENDED and STREAMING) have their process* method that permit them to be transitioned to another state.

the state REQUESTED_ACK has been removed, a new REQUESTING state has been introducted.
the state DEPROVISIONING_REQ has been removed.

currently, the deprovisioning method on the listeners is called by the processDeprovisioning, but it feels like it would be better to have it called by the DeprovisionRequestHandler

I had to implement a lease mechanism into InMemoryTransferProcessStore

I had to raise the provisionWait on AbstractQueueProvisioner to make DemoPushStreamTransferTest run on the CI, it was working locally but not on Actions. It was pretty flaky also before this PR. (e.g: https://github.com/eclipse-dataspaceconnector/DataSpaceConnector/runs/5478021065 on main branch)

Linked Issue(s)

#825 (not ready to be closed yet)

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)
  • linked GitHub project? (see the section on the right-hand side -->)

@ndr-brt
Copy link
Member Author

ndr-brt commented Mar 8, 2022

@algattik the minikube test is failing, but it works locally, do you know what the cause could be?

@ndr-brt
Copy link
Member Author

ndr-brt commented Mar 9, 2022

I think understood the problem, the FileTransferLocalSimulation does not wait for the file to be transferred, just for the transfer process to be in completed state (in the sample 04 there's no status checker so the manager puts the process in COMPLETED directly without checking if the file is transferred).
I added a StatusChecker to verify if the transfer is completed for real

@codecov-commenter
Copy link

codecov-commenter commented Mar 9, 2022

Codecov Report

Merging #831 (42479aa) into main (b5a5d34) will increase coverage by 0.56%.
The diff coverage is 71.54%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #831      +/-   ##
============================================
+ Coverage     60.07%   60.64%   +0.56%     
- Complexity     2262     2279      +17     
============================================
  Files           607      607              
  Lines         13071    13078       +7     
  Branches        912      912              
============================================
+ Hits           7853     7931      +78     
+ Misses         4792     4709      -83     
- Partials        426      438      +12     
Impacted Files Coverage Δ
.../spi/transfer/observe/TransferProcessListener.java 0.00% <0.00%> (ø)
...ansfer/demo/protocols/stream/DemoTopicManager.java 71.11% <50.00%> (-2.23%) ⬇️
...fer/store/memory/InMemoryTransferProcessStore.java 96.66% <66.66%> (-1.55%) ⬇️
...sfer/core/transfer/TransferProcessManagerImpl.java 75.84% <71.42%> (+31.69%) ⬆️
...tor/spi/types/domain/transfer/TransferProcess.java 73.72% <77.77%> (+1.06%) ⬆️
...re/command/handlers/DeprovisionRequestHandler.java 100.00% <100.00%> (ø)
...mo/protocols/common/AbstractQueuedProvisioner.java 83.33% <100.00%> (ø)
...i/types/domain/transfer/TransferProcessStates.java 100.00% <100.00%> (ø)

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 7fa65d3...42479aa. Read the comment docs.

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.

From your PR description:

currently, the provisioning method on the listeners is called by the processProvisioning, but it feels like it would be better to have it called by the DeprovisionRequestHandler

Is that a typo: provisioning should probably be deprovisioning, right? Assuming that's the case that would mean that the TransferProcessObservable "leaks" all the way through the ProvisionManager into the DeprovisionRequestHandler, which sounds awkward

@ndr-brt
Copy link
Member Author

ndr-brt commented Mar 9, 2022

@paullatzelsperger yes, it was a typo, I fixed it and also the other remarks, thanks

@ndr-brt
Copy link
Member Author

ndr-brt commented Mar 9, 2022

I had to implement a lease mechanism into the InMemoryTransferProcessStore, now everything work as expected, ready to review/merge

@ndr-brt
Copy link
Member Author

ndr-brt commented Mar 14, 2022

ready to review/merge

@ndr-brt
Copy link
Member Author

ndr-brt commented Mar 16, 2022

@paullatzelsperger @algattik is ok for you?

@ndr-brt
Copy link
Member Author

ndr-brt commented Mar 16, 2022

@jimmarino can this be merged?

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.

5 participants