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

TransferProcess: remove unnecessary REQUESTED_ACK state #673

Closed
ndr-brt opened this issue Feb 11, 2022 Discussed in #629 · 3 comments
Closed

TransferProcess: remove unnecessary REQUESTED_ACK state #673

ndr-brt opened this issue Feb 11, 2022 Discussed in #629 · 3 comments
Assignees
Milestone

Comments

@ndr-brt
Copy link
Member

ndr-brt commented Feb 11, 2022

I got no responses to the discussion about the REQUESTED_ACK state, so with issue I propose to delete it.

Discussed in #629

Originally posted by ndr-brt February 7, 2022
Going through the TransferProcess state machine I noticed this REQUESTED_ACK state.
This state, according to the code and the docs, can be reached only on the consumer side, and at the moment there's only one point where there's such state transition:
after the provision is complete, the TP is set in REQUESTED state and the data request is sent to the provider, if the response is successful, the TP is moved to REQUESTED_ACK and moved suddenly to IN PROGRESS (or STREAMING).

There's also a checkProvisioned() method that fetches TPs in such state to move them in IN PROGRESS (or STREAMING), but probably never won't find anything.

To follow the "state machine pattern" that was defined in #613 probably we would remove the REQUESTED_ACK state, and for the PROVISIONED TP the process should be:

  • put it in REQUESTED state
  • send data request to provider
  • if response is successful, move it to IN_PROGRESS or STREAMING
  • if response is failed, move it to PROVISIONED if there's a retry strategy set or to ERROR elsewhere.
@ndr-brt ndr-brt self-assigned this Feb 11, 2022
@ndr-brt ndr-brt added this to the Milestone 2 milestone Feb 11, 2022
@jimmarino
Copy link
Contributor

I'm not sure we can remove this. For example, after the ack is received from a request, there may be some time before the data transfer is put in progress (or streaming) on the provider side. Also, there is the case where the provider has to issue a key for the client to access the data and that happens sometime after the ack is received.

The code that transitions immediately from ACK to IN_PROGRESS/STREAMING was a temporary workaround until we implement the notification mechanism.

Maybe this is something we should look into further for M3?

@ndr-brt
Copy link
Member Author

ndr-brt commented Feb 22, 2022

Ok, seems reasonable, however this makes me feel like we have a discrepancy in the state naming convention.
At the moment there are these states:

  • REQUESTED (I'm about to call the provider)
  • REQUESTED_ACK (the provider responded successfully)

But, if I take a look at other cases e.g. in the *ContractNegotiationManagers, the pattern is (for example):

  • CONSUMER_OFFERING (I'm a about to call the provider)
  • CONSUMER_OFFERED (the provider responded successfully)

So maybe the two TransferProcess status should be called REQUESTING and REQUESTED, this would give more cohesion and reduce confusion.

@juliapampus juliapampus modified the milestones: Milestone 2, Milestone Scoping Feb 23, 2022
@ndr-brt
Copy link
Member Author

ndr-brt commented Mar 17, 2022

This was done in #831

@ndr-brt ndr-brt closed this as completed Mar 17, 2022
@juliapampus juliapampus modified the milestones: Milestone Scoping, Milestone 3 May 23, 2022
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

No branches or pull requests

3 participants