Remove topic partitions #170

rohitsalem · 2018-04-25T23:25:40Z

connects to ros2/ros2#476
Update design to specify the usage of the topic/service name directly without using the partitions.

mikaelarguedas · 2018-04-25T23:32:06Z

articles/140_topic_and_service_name_mapping.md

-For example, a plain topic called `/foo` would translate to a DDS partition `rt` and a DDS topic `foo`, which is the result of implicitly adding `/rt` to the namespace of a ROS topic which is in the root namespace `/` and has a base name `foo`, then the namespace getting the leading and trailing forward slashes (`/`) stripped.
-As another example, a topic called `/left/image_raw` would translate to a DDS partition `rt/left` and a DDS topic `image_raw`, which is the result of implicitly adding `/rt` to the namespace of a ROS topic which is in the namespace `/left` and has a base name `image_raw`, again stripping the leading and trailing forward slashes (`/`) from the namespace.
+For example, a plain topic called `/foo` would translate to a DDS topic `rt/foo`, which is the result of implicitly adding `/rt` to the namespace of a ROS topic which is in the root namespace `/` and has a base name `foo`.
+As another example, a topic called `/left/image_raw` would translate to a DDS topic `/rt/left/image_raw`, which is the result of implicitly adding `/rt` to the namespace of a ROS topic which is in the namespace `/left` and has a base name `image_raw`.


Do we add a slash in front of the prefix?
in this example /foo becomes rt/foo whereas /left/image_raw becomes /rt/left/image_raw

Based on the examples below, it looks like we don't add the leading / and /left/image_raw becomes rt/left/image_raw, is that correct?

Yes, we don't add the / to the prefixes, I'll change it

Karsten1987

a few comments

also make sure to put one sentence per line. This makes reviewing easier.

Karsten1987 · 2018-04-26T00:09:47Z

articles/140_topic_and_service_name_mapping.md

@@ -198,51 +198,17 @@ Any topic or service name that contains any tokens (either namespaces or a topic

 ## Mapping of ROS 2 Topic and Service Names to DDS Concepts

-The ROS topic and service name constraints allow more types of characters than the DDS topic names because ROS additionally allows the forward slash (`/`), the tilde (`~`), and the balanced curly braces (`{}`).
+The ROS topic and service name constraints allow more types of characters than the DDS topic names because ROS additionally allows the, the tilde (`~`), and the balanced curly braces (`{}`).


is the the , a typo or do you want to highlight the comma specifically?

Karsten1987 · 2018-04-26T00:18:29Z

articles/140_topic_and_service_name_mapping.md

-
-Since DDS partitions have no hierarchy or order to the array of strings, this proposal would use only one of the strings in the array to hold the entire ROS name's namespace.
-Therefore we preserve the forward slashes (`/`) that exist in the ROS name's namespace already.
+Previously forward slashes (`/`) were disallowed in DDS topic names, now the restriction has been lifted and therefore the ROS topic names are first prefixed with ROS Specific Namespace prefix (described below) are then mapped directly into DDS topic names.  


Can you reference any official statement or something indicating that it's now allowed?

Maybe the complete sentence can actually be part of the next section ROS Specific Namespace Prefix.

Regarding the official statement is there any issue or thread that refers to this? I think this is a general statement which says that ROS topics are mapped directly to DDS topics and therefore is here.

There is a DDS issue opened on this but hasn't been marked as "approved"/"resolved": https://issues.omg.org/issues/lists/dds-rtf5#issue-42236

If that's the only pointer we have I'll go ahead and include it

Karsten1987 · 2018-04-26T00:21:38Z

articles/140_topic_and_service_name_mapping.md

-Please bare in mind, that the length of the partition gets further diminished due to the introduction of a ROS specific prefix.
-The actual length of a ROS Topic, including the namespace hierarchy and the base name of the topic, may thus be varying in length as well.
-Yet, the base name token must not exceed the length of 256 characters as this is getting mapped directly as the DDS topic.
+The length of the DDS topic must not exceed 256 characters. Therefore the length of a ROS Topic, including the namespace hierarchy, the base name of the topic and any ros specific prefixes must not exceed 256 characters since this is mapped directly as DDS topic.


I think you should give some example service names.
Also make sure to reference the changes specifically for RTI here (contentfilter etc).
We should highlight that RTI add the GUID additional to the reply topic.

yes, should we also explain that we add Request/Reply to the service names in general (or is it mentioned somewhere else in the design doc) along with the special cases for RTI?

I don't think it's mentioned explicitly here, but I would actually be in favor of it. Also, it important to mention that RTI adds the GUID to it.

yes I added about RTI adding the GUID below

Karsten1987 · 2018-04-26T00:22:58Z

articles/140_topic_and_service_name_mapping.md

@@ -298,35 +251,29 @@ Therefore to allow ROS programs to interoperate with "native" DDS topic names th
 #### Examples and Ideas for Communicating with Non-ROS Topics

 Note that these are not part of the proposal, but only possible solutions to the issue of communicating with "native" DDS topics.
-This section should be update when/if a complete solution is found.
+This section should be updated when/if a complete solution is found.


I think you can actually update the design doc here as well, since we have the option to ignore the ROS conventions.

Now that we can use avoid_ros_namespace_convention Should I remove the alternatives of using the 'to have some markup in the scheme name' in the same section below? or leave it as is?

Having a markup to express the exact topic name is separate from having the ability to avoid applying our ROS specific mangling in our API. So I think it should not be removed because we have avoid_ros_namespace_convention.

Thanks, I'll have it listed as an alternative in the same section

Karsten1987 · 2018-04-26T00:23:40Z

articles/140_topic_and_service_name_mapping.md

+- Using the complete ROS name in the later strategy will cause a tighter length limit on base name because the DDS topic name would contain ROS prefix, namespace along with the base name which should not exceed DDS topic name limitation which is  256 characters.
+
+Rationale:
+- With the decision form the DDS vendors to allow forward slashes (`/`) in DDS topic names, using the complete ROS name seemed simple and more intuitive than using partitions.


typo: from

this typo is still there.

I think it is fixed here, this is an outdated version

Karsten1987 · 2018-04-26T00:25:41Z

articles/140_topic_and_service_name_mapping.md

+
+Trade-offs (in comparison to using the whole ROS name along with the namespaces):
+
+- Splitting the ROS name into "namespace" and "base name", and placing the complete namespace into a single partition seemed un-natural.


why unnatural?
Another reason for this is that newer standards such as XRCE DDS might not have partitions at all.

maybe there is a better word that un-natural. Placing the whole namespace with the prefix in a 'single' partition sounds like a workaround and therefore I added the term un-natural

Karsten1987 · 2018-04-26T00:26:26Z

articles/140_topic_and_service_name_mapping.md

+
+- Splitting the ROS name into "namespace" and "base name", and placing the complete namespace into a single partition seemed un-natural.
+- In general partitions are recommended to be used as a spare, but using partitions for all ROS names suggested otherwise.
+- Major concern was reported in this [issue](https://github.com/ros2/rmw_connext/issues/234), Where having two topics with same base name, although different namespace and different ypes caused problem. For example: topicA is `/camera/data` of type `Image` and topicB is `/imu/data` of type `Imu`. The base names for both topicA and topicB is `data`, generated errors as described in the [issue](https://github.com/ros2/rmw_connext/issues/234).


typoc: Where in caps

* unnecessary Now * XRCE DDS -> DDS-XRCE + link * try to make the sentence sound more naturel * attempt to clarify the 'un-natural' character of using partition * missing whitespace * avoid having same section and subsection title

ruffsl · 2018-04-30T23:55:50Z

articles/140_topic_and_service_name_mapping.md

+
+The length of service name has tighter limits than the length of the ROS Topics.
+In underlying RMW-DDS implementation, because the way in which services are implemented vary for different DDS vendors, and hence imposes the limit on actual service names that can be used.
+For example, in RTI connext, the service names are suffixed with the GUID value of the DDS participant, and a content filtered topic(max length 256 characters) is created which is mapped from the suffixed service name.


That's odd. Where can I read more up on this about RTI Connext? I'd think this runtime specific GUID suffix would disrupt efforts to statically define topic permissions for DDS security.

I believe that the suffix is not taken into account by Connext's builtin plugins.
That is why the current sros2 generated permission files work for Connext even if they dont have the postfix in them. Not sure how they handle it internally though..

Karsten1987

lgtm.

@mikaelarguedas Any further thoughts?

mikaelarguedas · 2018-05-15T01:35:22Z

articles/140_topic_and_service_name_mapping.md

+| ROS Name              | avoid_ros_namespace_conventions    | DDS Topic   |
+|-----------------------|------------------------------------|-------------|
+| `rostopic://image`    | `false`                            | `rt/image`  |
+| `rostopic://image`    | `true`                             | `image`     |


my understanding is that this will result in /image (based on discussion from ros2/rmw_fastrtps#192 (comment)).
We can leave this as is in the design doc if we ticket the missing bit that is to pass the user-specified topic name to rmw instead of the expanded one.
Otherwise we should correct it here

(same below)

yes that's correct I think it should be /image instead of image, as of now by default the topic name being passed here will already have a / appended to it (if / is not already present) be a fully qualified as ros topic isn't it?

ros2/ros2#496 has been created to track what is described here but not yet implemented

ruffsl · 2018-09-22T02:57:40Z

For posterity, was there a online discussion somewhere else that pertains to the reasoning behind this PR. I just want to share with someone else the context for this decision for the removal of partitions, and none of the connected PR here include the motivating description that I can cite to.

dhood · 2018-09-26T15:59:06Z

This has context behind the motivation: ros2/rmw_connext#234

Karsten1987 · 2018-09-27T07:36:49Z

@ruffsl The rationale for removing partitions is explained in this section of the design doc, which also links to the original reported issue on github:
http://design.ros2.org/articles/topic_and_service_names.html#alternative-using-dds-partitions

Do you think it's insufficient? We can reiterate over it if it's unclear.

spiderkeys · 2018-09-27T20:49:41Z

@Karsten1987 just as an outside opinion from a heavy user of both pure DDS and ROS, the rationale and write up were crystal clear and informative.

ruffsl · 2018-09-28T22:22:13Z

@Karsten1987 , the rationale explained in the design doc is sufficient. I should of have checked back with the updated docs again, but it good that it's linked here now as well.

rohitsalem added 2 commits April 20, 2018 18:24

removal of topic partitions, inital draft

f283d03

Updated alternative strategy of using partitions

c1a9e1e

rohitsalem added the in progress Actively being worked on (Kanban column) label Apr 25, 2018

mikaelarguedas reviewed Apr 25, 2018

View reviewed changes

Karsten1987 reviewed Apr 26, 2018

View reviewed changes

rohitsalem added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 26, 2018

rohitsalem mentioned this pull request Apr 26, 2018

Remove topic partitions ros2/ros2#476

Closed

fixed suggestions, added limits for service_names

6c41d4b

mikaelarguedas mentioned this pull request Apr 30, 2018

Mikael/remove topic partitions #171

Merged

Mikael/remove topic partitions (#171)

3ff8d1c

* unnecessary Now * XRCE DDS -> DDS-XRCE + link * try to make the sentence sound more naturel * attempt to clarify the 'un-natural' character of using partition * missing whitespace * avoid having same section and subsection title

ruffsl reviewed Apr 30, 2018

View reviewed changes

highlight rti connext limitations

09dfbd2

Karsten1987 approved these changes May 11, 2018

View reviewed changes

mikaelarguedas reviewed May 15, 2018

View reviewed changes

Karsten1987 merged commit 722aefe into gh-pages Jun 7, 2018

Karsten1987 deleted the remove_topic_partitions branch June 7, 2018 21:12

wjwwood removed the in review Waiting for review (Kanban column) label Jun 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove topic partitions #170

Remove topic partitions #170

rohitsalem commented Apr 25, 2018

mikaelarguedas Apr 25, 2018

rohitsalem Apr 25, 2018

Karsten1987 left a comment

Karsten1987 Apr 26, 2018

rohitsalem Apr 26, 2018

Karsten1987 Apr 26, 2018

rohitsalem Apr 26, 2018

mikaelarguedas Apr 26, 2018

rohitsalem Apr 26, 2018

Karsten1987 Apr 26, 2018

rohitsalem Apr 26, 2018 •

edited

Loading

Karsten1987 Apr 28, 2018

rohitsalem Apr 28, 2018

Karsten1987 Apr 26, 2018

rohitsalem Apr 26, 2018

wjwwood Apr 26, 2018

rohitsalem Apr 26, 2018

Karsten1987 Apr 26, 2018

Karsten1987 Apr 28, 2018

rohitsalem Apr 28, 2018

Karsten1987 Apr 26, 2018

rohitsalem Apr 26, 2018

Karsten1987 Apr 26, 2018

ruffsl Apr 30, 2018

mikaelarguedas May 1, 2018 •

edited

Loading

Karsten1987 left a comment

mikaelarguedas May 15, 2018 •

edited

Loading

rohitsalem May 15, 2018

mikaelarguedas May 29, 2018

ruffsl commented Sep 22, 2018

dhood commented Sep 26, 2018

Karsten1987 commented Sep 27, 2018

spiderkeys commented Sep 27, 2018

ruffsl commented Sep 28, 2018


		Trade-offs (in comparison to using the whole ROS name along with the namespaces):

		- Splitting the ROS name into "namespace" and "base name", and placing the complete namespace into a single partition seemed un-natural.

Remove topic partitions #170

Remove topic partitions #170

Conversation

rohitsalem commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Karsten1987 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

rohitsalem Apr 26, 2018 • edited Loading

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

mikaelarguedas May 1, 2018 • edited Loading

Choose a reason for hiding this comment

Karsten1987 left a comment

Choose a reason for hiding this comment

mikaelarguedas May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruffsl commented Sep 22, 2018

dhood commented Sep 26, 2018

Karsten1987 commented Sep 27, 2018

spiderkeys commented Sep 27, 2018

ruffsl commented Sep 28, 2018

rohitsalem Apr 26, 2018 •

edited

Loading

mikaelarguedas May 1, 2018 •

edited

Loading

mikaelarguedas May 15, 2018 •

edited

Loading