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

service registry as a built-in topic #415

Closed
18 of 25 tasks
budrus opened this issue Dec 8, 2020 · 29 comments · Fixed by #997, #1025, #917, #1034 or #1047
Closed
18 of 25 tasks

service registry as a built-in topic #415

budrus opened this issue Dec 8, 2020 · 29 comments · Fixed by #997, #1025, #917, #1034 or #1047
Assignees
Labels
enhancement New feature
Milestone

Comments

@budrus
Copy link
Contributor

budrus commented Dec 8, 2020

Brief feature description

The discovery requests from user side are currently transferred via the message queue connection. This has some drawbacks

  • The message queue is not only used for registration and creation of resources during startup but also during runtime
  • If an application polls the discovery information with a high frequency the message queue becomes a bottleneck
  • We have a message size limitation on the message queue or unix domain socket, i.e. we need some transport protocol to split the response to several messages if there are a lot services. Currently it is a limitation as we do not have such a protocol
  • The findService() is very close to AUTOSAR ara::com and maybe cannot be used in a reasonable way in other frameworks
  • If someone wants to use ara::com service discovery callbacks, having only a polling based interface is a big drawback

Detailed information

The proposal is to have a service registry that provides it's content as a built-in topic, similar to the introspection topics. Then a subscriber could be created to get the information either in a polling way or event-driven by using the subscriber in combination with a waitset. The service registry publisher would use a history with size 1. Then every subscriber gets the current state on subscribe if desired. By providing the full CaPro ID information and not only service and instance IDs, this built-in topic could also be used for non AUTOSAR use cases. With this approach having a large service registry is fine as we don't have to send the content over the message queue and it's distributed with iceoryx' finest true zero-copy. I guess we can then also get rid of getServiceRegistryChangeCounter() which smells like workaround

Todo

Discussions:

  • Consider ServiceRegistry::find("Foo", Wildcard, [] (auto& serviceDescription) { }) API
  • Can ServiceDescription::isValid() be avoided?
  • When ManyToManyPolicy is used, would a user be interested in the number of offers? This is stored in the ServiceRegistry as ServiceDescriptionEntry::referenceCounter but currently not passed up to the user API.
  • Does the ServiceRegistry belong to roudi or runtime? Somehow, it sits in-between, currently it is in source/roudi/service_registry.cpp Moved to Refactor ServiceDiscovery #1200

Stretch goals:

@elBoberido
Copy link
Member

A big drawback to the built-in topic is the worst case memory consumption. We maybe have to take into account that each application holds a different generation of the service registry sample, depending on how it's implemented. Even if we don't expose the sample to the user and do everything in the runtime, there might still be situation where multiple generations are held by various runtimes. With the introspection we currently assume that there is only one client and we just print some errors if the mempool runs out of chunks. It might be problematic to do the same with the service registry topic.

The proposed fix for #332 will enable segmented transmissions by IPOSIX PC channels. This doesn't solve the problem of the shared communication channel for registration, heartbeat and requesting resources like ports. I would split that into multiple channels, at least one just for registration/heartbeat and resource requests. This would still not solve the problem of sending large data by this channels but at least doesn't block registration. The IPC channel could be used as a fallback in case all chunks reserved for the service registry are exhausted. In order to make this work, the service registry must not be a built-in topic but still be requested by the IPC channel. So we would have the following scenario:

  1. app requests the service registry data
  2. RouDi has this information stored in a chunk and there is a spare chunk to use for new service registry data
    • the chunk is sent as relative pointer to the app by the IPC channel
    • the spare chunk is used as new storage for the service registry data
  3. RouDi has this information stored in a chunk, but there is no spare chunk to use for service registry changes
    • RouDi serializes the data from the chunk and sends it to the app by the IPC channel

This ensures that the application always gets the information although it might be expensive if the chunks are exhausted. Alternatively instead of serializing the chunk, a pending response or error message could be sent.

@budrus
Copy link
Contributor Author

budrus commented Dec 9, 2020

good point @elBoberido. Having the MQ or UDS channel as point of contact and in the startup phase of an application is fine. But during runtime any possible interference from such a resource and potentially blocking calls on it should be avoided. I'm not sure if we solve this with your proposal without making things complicated.
The "how many chunks would we need" question is valid. But before having a second request response channel via MQ I would try to solve it with iceoryx pub/sub or req/res
But I see that we need some more discussions

@elBoberido
Copy link
Member

sure, iceoryx req/res would work exactly like the POSIX IPC channel, just that there would be a pending response/error response instead of serialization

@budrus
Copy link
Contributor Author

budrus commented Dec 15, 2020

When we touch the service registry here, should we also take care about naming of our IDs in the CaPro description? The proposal of @MatthiasKillat was to have Topics, Instances of Topics and Groups of Topics. I.e. renaming ServiceID to GroupID, InstanceID stays, renaming EventID to TopicID.
Then we could define what capabilities our service registry has (e.g. get all instances of a specific topic, or get all topics and instances of a specific group. And for sure the documentation and mapping to ara::com, DDS and ROS
Sure we should come up with several PRs but maybe it makes sense to have it in this issue or would you prefer somehow linked individual issues?

@mossmaurice @MatthiasKillat

@mossmaurice
Copy link
Contributor

@budrus I think it's a good idea to rename the three CaPro identifiers. We need to make sure to have good documentation, so that developers coming from different backgrounds feel at home. Ok from my side to do this in this issue.

@budrus budrus added this to the Prio 2 milestone Jan 5, 2021
@orecham
Copy link
Contributor

orecham commented Jan 9, 2021

With the proposed change to the composition of a service description, we would be restricting services to a single topic. Is this something that we want ?
Furthermore, I don't think it makes sense to have "multiple instances of a topic", which is a characteristic introduced by the proposal. To me, it makes more sense to have a different topic per instance.

Considering the above, it seems that the concepts of services and events are being conflated, and this makes it confusing (at least to me).

What are we trying to achieve with the change?

  • ability to group together services ?
  • a more "generic" abstraction ?

If the former, then I think just adding another component groupId would be sufficient.
If the latter, then I question why. I think the idea of services is pretty widespread and understandable. If wanting to go ahead with the different abstraction anyway, then I think the instance ID should be removed and instead be a part of the topic.
If both, then the proposal doesn't work and should be revised.

@budrus @MatthiasKillat

@mossmaurice mossmaurice assigned mossmaurice and unassigned budrus Mar 9, 2021
mossmaurice added a commit to ApexAI/iceoryx that referenced this issue Apr 26, 2021
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
mossmaurice added a commit to ApexAI/iceoryx that referenced this issue May 6, 2021
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
mossmaurice added a commit to ApexAI/iceoryx that referenced this issue May 7, 2021
…relevant code places

Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
mossmaurice added a commit to ApexAI/iceoryx that referenced this issue May 7, 2021
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
mossmaurice added a commit to ApexAI/iceoryx that referenced this issue May 7, 2021
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
mossmaurice added a commit to ApexAI/iceoryx that referenced this issue May 10, 2021
…uilt-in topic

Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
mossmaurice added a commit to ApexAI/iceoryx that referenced this issue May 10, 2021
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
mossmaurice added a commit to ApexAI/iceoryx that referenced this issue May 10, 2021
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
mossmaurice added a commit to ApexAI/iceoryx that referenced this issue May 11, 2021
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
FerdinandSpitzschnueffler added a commit to ApexAI/iceoryx that referenced this issue Feb 18, 2022
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
FerdinandSpitzschnueffler added a commit to ApexAI/iceoryx that referenced this issue Feb 18, 2022
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
FerdinandSpitzschnueffler added a commit to ApexAI/iceoryx that referenced this issue Feb 18, 2022
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
FerdinandSpitzschnueffler added a commit to ApexAI/iceoryx that referenced this issue Feb 18, 2022
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
dkroenke pushed a commit to ApexAI/iceoryx that referenced this issue Feb 18, 2022
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
dkroenke pushed a commit to ApexAI/iceoryx that referenced this issue Feb 18, 2022
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
dkroenke pushed a commit to ApexAI/iceoryx that referenced this issue Feb 18, 2022
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
dkroenke pushed a commit to ApexAI/iceoryx that referenced this issue Feb 18, 2022
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
dkroenke pushed a commit to ApexAI/iceoryx that referenced this issue Feb 18, 2022
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
dkroenke added a commit to ApexAI/iceoryx that referenced this issue Feb 18, 2022
Signed-off-by: Dietrich Krönke <dietrich.kroenke@apex.ai>
mossmaurice added a commit to ApexAI/iceoryx that referenced this issue Feb 19, 2022
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
mossmaurice added a commit to ApexAI/iceoryx that referenced this issue Feb 19, 2022
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
FerdinandSpitzschnueffler added a commit to ApexAI/iceoryx that referenced this issue Feb 21, 2022
…nition

Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
FerdinandSpitzschnueffler added a commit to ApexAI/iceoryx that referenced this issue Feb 21, 2022
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
FerdinandSpitzschnueffler added a commit to ApexAI/iceoryx that referenced this issue Feb 21, 2022
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
FerdinandSpitzschnueffler added a commit that referenced this issue Feb 21, 2022
@mossmaurice
Copy link
Contributor

User API finished, implementation will be refactored with #1200

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment