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.

Sign up for GitHub

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

Jump to bottom

Avoid unnecessary sets of GUID_t #586

Open
wants to merge 2 commits into
base: rolling
Choose a base branch
from
Open

Avoid unnecessary sets of GUID_t #586

wants to merge 2 commits into from

Conversation

MiguelCompany
Copy link
Collaborator

While debugging the test failures on ros2/ros2#1241, I discovered we could directly keep the publisher and subscription counts instead of keeping a set of GUID_t.

This PR does just that.

@MiguelCompany MiguelCompany mentioned this pull request Mar 1, 2022
Merged
clalancette
clalancette approved these changes Mar 1, 2022
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. I'll go ahead and run CI on it.

@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

fujitatomoya
fujitatomoya reviewed Mar 1, 2022
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

@MiguelCompany this PR looks good to me (using current count from dds), but i am not sure why this can fix the problem. Using GUID list based on handles, why it cannot work in the same way with this PR?

@clalancette
Copy link
Contributor

We can ignore the Windows failure; that is due to a separate issue that has since been fixed. We can rerun that later.

However, there are a lot of new failures in Linux and Linux aarch64 with this PR in place. @MiguelCompany can you take a look?

@MiguelCompany
Copy link
Collaborator Author

@clalancette I only tested this against the master branch of Fast DDS, as I was debugging ros2/ros2#1241.

It seems current_count is not working properly on branch 2.3.x. I can leave this open till we have fixed that on Fast DDS 2.3.x, so it can later be backported into Galactic.

After we change rolling to use Fast DDS master though, I would use a different approach I have on a local branch, in which the DataReader and the DataWriter are directly queried for the match count.

@clalancette
Copy link
Contributor

@MiguelCompany Now that we have ros2/ros2#1241 merged, feel free to revise this PR to what you think it should be, or open a new one with your new approach. Then we can run CI again.

@MiguelCompany
Copy link
Collaborator Author

@clalancette Thanks. I've just pushed the new approach, in which the matched counts are directly obtained from Fast DDS.

@MiguelCompany
Copy link
Collaborator Author

MiguelCompany commented May 26, 2022 •
edited
Loading

@clalancette Friendly ping

Sorry, I just saw this has conflicts. Will rebase tomorrow

@clalancette clalancette self-assigned this Jun 9, 2022
@audrow audrow changed the base branch from master to rolling June 28, 2022 14:22
@fujitatomoya
Copy link
Collaborator

@MiguelCompany can you rebase this when you have time?

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

@clalancette clalancette clalancette approved these changes

@EduPonz EduPonz EduPonz approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees

@clalancette clalancette

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MiguelCompany @clalancette @fujitatomoya @EduPonz