Update uDiscovery specs #118
Conversation
|
|
||
| // Query the database to fetch a list of 1 or more properties for a given node. | ||
| rpc FindNodeProperties(FindNodePropertiesRequest) returns (FindNodePropertiesResponse) { | ||
| // Retrieve the list of uEs under a device. |
There was a problem hiding this comment.
What would a uE need to call this API? What is the purpose of this API?
There was a problem hiding this comment.
GetuETopics could be used to fetch the list of topics serviced by a given uE. It is a focused API to give that information as opposed to FindUE which gives everything about uE.
|
|
||
| // Register to receive Notifications to changes to one of more Nodes. The changes | ||
| // are published on the notification topic: '/core.udiscovery/3/nodes#Notification' | ||
| rpc RegisterForNotifications(NotificationsRequest) returns (uprotocol.v1.UStatus) { |
There was a problem hiding this comment.
Are there no more notifications by udiscovery (when a new service is added or removed)?
| === Read/Query APIs | ||
|
|
||
| Query APIs are used to lookup content in the database, either to resolve URIs (to be used by applications) or to fetch content of a database. | ||
|
|
||
| * Any uE *MAY* call the query APIs defined in the sections below | ||
| * *MUST NOT* return Nodes that are flagged as `inactive` | ||
| * Remote Nodes that have `expired` *MUST* be refreshed to the CDS | ||
| * Remote Nodes that are `expired` *MUST* be refreshed to the CDS |
There was a problem hiding this comment.
All node references need to be removed
up-l3/udiscovery/v3/README.adoc
Outdated
| `ResolveUri()` is used to lookup names from ids or vice versa meaning to go to/from Long Uri from/to micro Uri. | ||
|
|
||
| For portability between SDKs, Long Form URIs shall be serialized to String and Short form Uris shall be serialized to Bytes per link:../../../basics/uri.adoc[URI Specifications]. | ||
| * *MUST* update `last_accessed` Node attribute when API is called |
There was a problem hiding this comment.
No longer relevant for the API.
|
|
||
| ==== Adding Node(s) | ||
| ==== Get UE Topics |
There was a problem hiding this comment.
Please add more details as to why this APi exists, who would use it when
|
|
||
|
|
||
| == Failure Recovery | ||
|
|
||
| In the event that the databases between the CDS and LDS becomes out of sync, the discovery service components (uLDS, uDDS, uCDS) *MAY* fetch the contents using `FindNodes()` API. | ||
| In the event that the databases between the CDS and LDS becomes out of sync, the discovery service components (uLDS, uDDS, uCDS) *MAY* fetch the contents using read APIs. |
There was a problem hiding this comment.
Database specific detail that is out of scope for this API documentation.
There was a problem hiding this comment.
the other new APIs in the proto are not covered here and there is no explanation about their purpose.
Co-authored-by: Steven Hartley <steven@orangepeel.ca>
| } | ||
| /* | ||
| UEs shall call RegisterUE API to add themselves to discovery. | ||
| */ |
There was a problem hiding this comment.
Please provide more description for these APIs as I do not understand their purpose. Also we are revamping the UUri (please watch up-spec project to see what is changing). comments in method definitions cannot have spaces between them for documentation generation purposes.
| option (method_id) = 4; | ||
| } | ||
| /* | ||
| UEs shall call UnregisterUE API to remove themselves from discovery. |
There was a problem hiding this comment.
What does registering and unregistering do? Is this something that a uE actually does? How do you ensure that we don't call the API to spoof being someone else?
| UEs shall call SetUETopics API to update discovery with the list of topics they service. | ||
| This will be used in two scenarios: | ||
| 1. when a UE is added on a device | ||
| 2. when UE wants to udpate the topic list |
There was a problem hiding this comment.
Perhaps we register and pass the list of all the configured topics in one command.
|
|
||
| /* | ||
| UEs shall call SetUEProperties API to update discovery with updated property values. | ||
| For example, this could be used to update an UE's version. |
There was a problem hiding this comment.
properties are part of the UUri, what other properties are we setting? Perhaps we need an explicit structure like UServiceTopic but more a UServiceProperties.
| // uProtocol formatted URI for the node to search, ex. '//vcu.VIN/core.app.hartley' | ||
| // shall return the 'core.app.hartley' node | ||
| string uri = 1; | ||
| /* |
There was a problem hiding this comment.
Why not call the GetUETopics() passing a wildcard?
| } | ||
| /* | ||
| UpdateData is NOT a customer facing API. This shall be used discovery | ||
| instances to push an update between to core components (streamer)/ other discovery instances. |
There was a problem hiding this comment.
Then do not put this here. Also Streamer should not be updating information in the discovery database.
| message FindNodePropertiesRequest { | ||
| // the uri for the node in the database | ||
| string uri = 1; | ||
| rpc SyncData(UpdateDataRequest) returns (status){ |
There was a problem hiding this comment.
above it is called updateData and here it is called SyncData()
| uprotocol.v1.UStatus status = 2; | ||
| } | ||
|
|
||
| /* All response messages shall include uprotocol status field. This shall indicate whether |
There was a problem hiding this comment.
We are saying now in #112 that we do not want to also pass this in the reply but the RPC returns the status per commstatus (of response umessage)
This PR is to make updates to uDiscovery specifications to make APIs more intent revealing and to removed unwanted complexities.
#137