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

Move action to payload #4

Open
altavir opened this issue Jul 2, 2020 · 9 comments · May be fixed by scientific-software-hub/magix-tango-connector#8
Open

Move action to payload #4

altavir opened this issue Jul 2, 2020 · 9 comments · May be fixed by scientific-software-hub/magix-tango-connector#8

Comments

@altavir
Copy link
Member

altavir commented Jul 2, 2020

The action seems to be payload-specific so it probably should be moved to the payload.

@Ingvord
Copy link
Member

Ingvord commented Jul 2, 2020

Yes and no, consider read action - that is definitely will be implemented in every upstream endpoint connector. I foresee action dispatcher that depending on the action creates a kind of ActionExecuter and injects payload into it. Otherwise payload becomes predefined structured object like:

{
  "payload":{
     "action":"string",
     "data": []
   }
}

I do not see any justification for that structural complication at the moment just for one field.

@altavir
Copy link
Member Author

altavir commented Jul 2, 2020

OK, keep it open please. I will play with the format a little bit more to understand, how it should work.

@altavir
Copy link
Member Author

altavir commented Jul 19, 2020

OK, let's leave it external, but we need to specify at least basic actions that should be recognized and supported. I propose the following:

  • read: payload contains names of properties to be read. The expected response is a message with array payload with the same names. If one of the read operations fails, the whole operations return failure.
  • write: payload contains names and values of properties to be written. The response may or may not be present but in general contains names and values of properties being set.
  • execute: run an action or several actions. The request contains the action name and the argument. The response contains an action name and the result (argument and result both could be empty.
  • [Optional] propertyList: no payload in the request. The response contains the list of supported properties and their descriptors.
  • [Optional] actionList: no payload in the request. The response contains the list of supported actions and their descriptors.

@Ingvord
Copy link
Member

Ingvord commented Jul 22, 2020

There are two more reasons to keep it up level:

  • security monitoring
  • logging

For instance, pair of user and action can be monitored by an intermediate service to perform security check, or logging.

It seems, this conflicts with an array payload though. I would suggest to revert to a single element payload. We can still group messages on end point level using Rx for optimizing requests to hardware e.g. buffer().groupBy(action).subscribe()

@altavir
Copy link
Member Author

altavir commented Jul 22, 2020

Actually, I see only one case for array payload - when multiple device properties are changed in a short time. In this case, the array still could exist, but since it is system-specific, it will be inside the payload like:

{
  user: "someUser",
  payload: {
    device: "deviceName",
    properties: [
      {
         name: ...,
         value:
      },
      ...
    ]
 }
}

@Ingvord
Copy link
Member

Ingvord commented Jul 22, 2020

I have updated RFC-1 accordingly, see 1e613cb

@altavir
Copy link
Member Author

altavir commented Jul 22, 2020

OK

@altavir
Copy link
Member Author

altavir commented Nov 6, 2020

I had to reopen the issue because after playing for some time with Tango to DF converter, I found out that there is no reasonable way to use tango-base actions outside of tango. I've found out that it is quite easy to move action to payload in magix-tango-connector. So I suggest that this change should be made in RFC.

@Ingvord
Copy link
Member

Ingvord commented Nov 6, 2020

This will also require changes in JS and Python clients

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 a pull request may close this issue.

2 participants