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)
|
Closing this pull request as it was replaced with #219 |
This PR is to make updates to uDiscovery specifications to make APIs more intent revealing and to removed unwanted complexities.
#137