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

remove partitions #45

Merged
merged 7 commits into from
Jun 8, 2018
Merged

remove partitions #45

merged 7 commits into from
Jun 8, 2018

Conversation

mikaelarguedas
Copy link
Member

This moves the hack for default created topics and services from partition to topic names to match the code changes from ros2/ros2#476.

Note that this now enforces that all created topics have an empty partition

Connects to ros2/ros2#476

fix topics/srvices prefixes
@mikaelarguedas mikaelarguedas added the in progress Actively being worked on (Kanban column) label Apr 30, 2018
@mikaelarguedas mikaelarguedas requested a review from dhood May 31, 2018 17:09
@mikaelarguedas mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels May 31, 2018
dhood added 2 commits June 1, 2018 15:29
The double <tag> surprised me at first
Copy link
Member

@dhood dhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a couple of small changes so the code is easier to read. I think it's reasonable now that it doesn't need to deal with the two tag types, but feel free to revert them if the changes weren't appropriate

</partitions>
<topics>
<topic>%s</topic>
</topics>
</%s>
""" % (tag, 'rt', topic_name, tag)
""" % (tag, 'rt/' + topic_name, tag)
# TODO(mikaelarguedas) remove this hardcoded handling for default parameter topics
# TODO(mikaelarguedas) remove the need for empty partition
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this todo can be removed, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, removed in 9a622e6

for key, value in service_topic_prefixes.items():
if key == 'Request':
for service_direction, topic_prefix in service_topic_prefixes.items():
if service_direction == 'Request':
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into that. I couldnt find a "good" name when I refactored so I didnt change it.

I don't think that "service_direction" is what we mean here. For example, a service server would subscribe to a Request topic.

I made another attempt at renaming the variable in 70bc265 trying to match the "topic_prefix" variable introduced here. Feedback welcome

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, good point, that name looks good!

@mikaelarguedas
Copy link
Member Author

merging this as all dependeant PRs have been merged.

Access control is currently not working since we introduced the parameter servers starting automatically with the node. As this is out of the scope of this PR I will address it in a follow-up

@mikaelarguedas mikaelarguedas merged commit b395595 into master Jun 8, 2018
@mikaelarguedas mikaelarguedas deleted the remove_partitions branch June 8, 2018 10:59
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Jun 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants