uDiscovery Redesign #219

stevenhartley · 2024-08-02T19:39:04Z

The following is the redesign of uDiscovery service based on the various lessons learned from deploying uDiscovery in production and valuable feedback from developers. uDiscovery is no longer a dumping ground for all kinds of information but instead only stores service discovery information (service location, version, and service topic metadata).

#137

kz9nyy

Looks good to me.
My only doubts are regarding the reconciliation process.
I don't have a clear understanding after reading the API design.
I think some further explanation would help someone who is reading this for the first time.

up-l3/udiscovery/v3/service.adoc

kz9nyy · 2024-08-08T15:50:49Z

up-l3/udiscovery/v3/service.adoc

+
+Domain-UDiscovery ->> Local-UDiscovery: GetServiceTopics()
+Local-UDiscovery -->> Domain-UDiscovery: GetServiceTopicsResponse
+Domain-UDiscovery ->> Domain-UDiscovery: ReconcileData()


I understand the ReconcileData function will re-sync the DB.
What do you compare against? How do you test for equality?
In this example, will it just re-sync the topics i.e. (partial sync)?
I'm thinking that you would need a snapshot or hash of the entire DB to guarantee synchronization.

I wonder if that is implementation details for a given implementation or something that we need to specify here. @kz9nyy Could we elaborate on the reconcileData() functionality in a subsequent PR?

up-l3/udiscovery/v3/service.adoc

up-l3/udiscovery/v3/client.adoc

sophokles73 · 2024-08-13T07:15:32Z

up-l3/udiscovery/v3/client.adoc

+
+== Client Facing APIs
+
+The client facing APIs of uDiscovery provide the means for uEs to discover services (where they are located, versions, etc...), and topic metadata that a service produces (method id and permissions, publish topic message formats, etc...). The API is declared in the the xref:../../../up-core-api/uprotocol/core/udiscovery/v3/udiscovery.proto[uDiscovery service proto].


You may want to consider introducing a document attribute for the reference to the proto file. This will make it easier to maintain the link (if it needs to change) and will save you a lot of typing :-)

I saw that in another change you did, I'll consider it for the future edits

up-l3/udiscovery/v3/service.adoc

sophokles73 · 2024-08-13T07:19:23Z

up-l3/udiscovery/v3/service.adoc

+  - *ue_id:* The unique identifier of the service (both the instance and service ID portions).
+  - *authority_name:* The name of the authority that hosts the service.
+  - *ue_version_major:* The version of the service.
+  - *resource_id:* The resource ID (value of 0 us used to indicate the default ID for the service).


what is the default ID for a service?

Resource_id of 0 means the default topic for responses and notifications to be sent to

sophokles73 · 2024-08-13T07:21:03Z

up-l3/udiscovery/v3/service.adoc

+
+== Client APIs
+
+The following section we will elaborate on the service-side requirements for the uDiscover service when it serves the client facing APIs defined in the xref:../../../up-core-api/uprotocol/core/udiscovery/v3/udiscovery.proto[udiscovery.proto].


you might want to introduce a document attribute for the link to the proto ...

up-l3/udiscovery/v3/service.adoc

up-core-api/uprotocol/core/udiscovery/v3/udiscovery.proto

sophokles73 · 2024-08-13T07:38:00Z

up-core-api/uprotocol/core/udiscovery/v3/udiscovery.proto

-  // 3. code.INVALID_ARGUMENT: The passed UUri is invalid
-  // 4. code.PERMISSION_DENIED: The caller does not have permission to perform the query
-  rpc LookupUri(LookupUriRequest) returns (LookupUriResponse) {
+  // Find Services. 


FMPOV all of this documentation should go into the specification document ...

@sophokles73 I changed this now so please re-review

mishap4 · 2024-08-16T20:43:10Z

up-core-api/uprotocol/core/udiscovery/v3/udiscovery.proto

+  // How long the topic metadata is valid for in seconds.
+  // If the metadata is older than this value, the client should re-fetch the metadata.
+  // If the field is missed or the value is 0, the client should assume the metadata is valid forever.
+  optional uint32 ttl = 3;


ServiceTopicInfo already includes ttl, why this one is needed?

Correct, I will remove it now

mishap4 · 2024-08-16T20:51:55Z

up-core-api/uprotocol/core/udiscovery/v3/udiscovery_replicator.proto

+  // If the metadata has expired, the UDiscovery service that received this data must flush the ServiceTopic info.
+  // If the field is missed, the ServiceTopicInfo is valid forever.
+  // If the field is set to 0, the ServiceTopicInfo should be removed from the database immediately.
+  optional uint32 ttl = 2;


Removal is not obvious when ttl is used to request that. Default value 0 by generated code is returned if field is not set.

@mishap4 , Because the field is optional, there is a difference from default and not present, this means we can treat them differntly.

up-core-api/uprotocol/core/udiscovery/v3/udiscovery.proto

sophokles73 · 2024-08-19T07:58:43Z

up-core-api/uprotocol/core/udiscovery/v3/udiscovery.proto

@@ -42,43 +42,22 @@ service uDiscovery {

  // Find Services. 
  //
-  // The API is used to find the authority(ies), instance(s), and version(s) of services based on the passed URI.
+  // Discovery the authority(ies) (where), instance(s), and version(s) of services based on the passed URI.


Did you mean

Discovery of the authorities ...

?

Meant to be one or many authories but I guess I can just make it all plural.

up-l3/udiscovery/v3/client.adoc

sophokles73 · 2024-08-19T08:04:11Z

up-l3/udiscovery/v3/client.adoc

 --
+* *MUST* return a `UCode.INVALID_ARGUMENT` if: 
+  * The lower 16 bits of `ue_id` (_service ID_) is missing.


how can the lower 16 bits be missing, i.e. how would a corresponding URI look like? Or did you mean: if the lower 16 bits are 0x0000?

hum... I guess you might want to find out version for the uSubscription service so this requirement needs to be removed then all together.

Can you add a corresponding entry to the list of examples? I still haven't understood the use case ...

Pushing a change now

sophokles73 · 2024-08-19T08:05:17Z

up-l3/udiscovery/v3/client.adoc

+
+==== Example Use Cases
+
+ Below are example use cases for the API when passed certain URIs (shown in string form) along with the


this sentence seems to be missing an ending

up-l3/udiscovery/v3/client.adoc

up-l3/udiscovery/v3/service.adoc

stevenhartley · 2024-09-19T14:33:41Z

up-core-api/uprotocol/core/udiscovery/v3/udiscovery.proto

@@ -42,43 +42,22 @@ service uDiscovery {

  // Find Services. 
  //
-  // The API is used to find the authority(ies), instance(s), and version(s) of services based on the passed URI.
+  // Discovery the authority(ies) (where), instance(s), and version(s) of services based on the passed URI.


Meant to be one or many authories but I guess I can just make it all plural.

stevenhartley · 2024-09-19T14:34:14Z

up-core-api/uprotocol/core/udiscovery/v3/udiscovery.proto

+  // How long the topic metadata is valid for in seconds.
+  // If the metadata is older than this value, the client should re-fetch the metadata.
+  // If the field is missed or the value is 0, the client should assume the metadata is valid forever.
+  optional uint32 ttl = 3;


Correct, I will remove it now

stevenhartley · 2024-09-19T14:36:28Z

up-l3/udiscovery/v3/client.adoc

 --
+* *MUST* return a `UCode.INVALID_ARGUMENT` if: 
+  * The lower 16 bits of `ue_id` (_service ID_) is missing.


hum... I guess you might want to find out version for the uSubscription service so this requirement needs to be removed then all together.

up-l3/udiscovery/v3/client.adoc

sophokles73

LGTM

sophokles73 · 2024-09-24T08:19:52Z

@stevenhartley can you rebase this PR?

The following is the redesign of uDiscovery service based on the various lessons learned from deploying uDiscovery in production and valuable feedback from developers. uDiscovery is no longer a dumping ground for all forms of information but instead only stores service discovery information (service location, version, and service topic metadata). Co-authored-by: Christian Alexander <christian.alexander101@gmail.com> Co-authored-by: Kai Hudalla <sophokles.kh@gmail.com> #137

stevenhartley added the enhancement New feature or request label Aug 2, 2024

stevenhartley added this to the v1.6.0-alpha.4 milestone Aug 2, 2024

stevenhartley requested a review from sophokles73 August 2, 2024 19:39

kz9nyy reviewed Aug 8, 2024

View reviewed changes

stevenhartley mentioned this pull request Aug 9, 2024

Update uDiscovery specs #118

Closed

stevenhartley self-assigned this Aug 9, 2024

kz9nyy suggested changes Aug 9, 2024

View reviewed changes

up-l3/udiscovery/v3/service.adoc Outdated Show resolved Hide resolved

up-l3/udiscovery/v3/service.adoc Outdated Show resolved Hide resolved

kz9nyy approved these changes Aug 9, 2024

View reviewed changes

sophokles73 reviewed Aug 13, 2024

View reviewed changes

stevenhartley requested a review from sophokles73 August 15, 2024 21:33

mishap4 reviewed Aug 16, 2024

View reviewed changes

sophokles73 reviewed Aug 19, 2024

View reviewed changes

stevenhartley commented Sep 19, 2024

View reviewed changes

sophokles73 approved these changes Sep 24, 2024

View reviewed changes

stevenhartley closed this by deleting the head repository Sep 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

uDiscovery Redesign #219

uDiscovery Redesign #219

stevenhartley commented Aug 2, 2024 •

edited

Loading

kz9nyy left a comment

kz9nyy Aug 8, 2024

stevenhartley Aug 9, 2024

sophokles73 Aug 13, 2024

stevenhartley Aug 15, 2024

sophokles73 Aug 13, 2024

stevenhartley Aug 15, 2024

sophokles73 Aug 13, 2024

sophokles73 Aug 13, 2024

stevenhartley Aug 16, 2024

mishap4 Aug 16, 2024

stevenhartley Sep 19, 2024

mishap4 Aug 16, 2024

stevenhartley Sep 19, 2024

sophokles73 Aug 19, 2024

stevenhartley Sep 19, 2024

sophokles73 Aug 19, 2024

stevenhartley Sep 19, 2024

sophokles73 Sep 23, 2024

stevenhartley Sep 24, 2024

sophokles73 Aug 19, 2024

stevenhartley Sep 19, 2024

stevenhartley Sep 19, 2024

stevenhartley Sep 19, 2024

sophokles73 left a comment

sophokles73 commented Sep 24, 2024


		== Client Facing APIs

		The client facing APIs of uDiscovery provide the means for uEs to discover services (where they are located, versions, etc...), and topic metadata that a service produces (method id and permissions, publish topic message formats, etc...). The API is declared in the the xref:../../../up-core-api/uprotocol/core/udiscovery/v3/udiscovery.proto[uDiscovery service proto].


		== Client APIs

		The following section we will elaborate on the service-side requirements for the uDiscover service when it serves the client facing APIs defined in the xref:../../../up-core-api/uprotocol/core/udiscovery/v3/udiscovery.proto[udiscovery.proto].


		==== Example Use Cases

		Below are example use cases for the API when passed certain URIs (shown in string form) along with the

uDiscovery Redesign #219

uDiscovery Redesign #219

Conversation

stevenhartley commented Aug 2, 2024 • edited Loading

kz9nyy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophokles73 left a comment

Choose a reason for hiding this comment

sophokles73 commented Sep 24, 2024

stevenhartley commented Aug 2, 2024 •

edited

Loading