Skip to content

Use unique mangled names when creating Content Filter Topics - #762

Merged
ahcorde merged 2 commits into
ros2:rollingfrom
eProsima:fix/rolling/unique-cft-names
Jun 3, 2024
Merged

Use unique mangled names when creating Content Filter Topics#762
ahcorde merged 2 commits into
ros2:rollingfrom
eProsima:fix/rolling/unique-cft-names

Conversation

@Mario-DL

@Mario-DL Mario-DL commented May 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR makes every Content Filter Name unique by adding a static atomic counter. With this change, two or more content-filtered subscriptions can be created for the same topic name.

Tests are included in the following related PRs

@Mario-DL
Use unique mangled names when in CFTs
ec6424a 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Mario-DL requested review from gbiggs and sloretz as code owners May 30, 2024 10:04
This was referenced May 30, 2024
Merged
Merged
Closed
ahcorde
ahcorde requested changes May 30, 2024
View reviewed changes

@ahcorde ahcorde left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Check linters cpplint and uncrustify

Comment thread rmw_fastrtps_shared_cpp/src/utils.cpp
@Mario-DL
Rev changes
92fadee 
Signed-off-by: Mario-DL <mariodominguez@eprosima.com>
@fujitatomoya

fujitatomoya commented May 31, 2024

Copy link
Copy Markdown
Collaborator

@Mario-DL thanks for the PR.

i would like to clarify that,

This PR makes every Content Filter Name unique by adding a static atomic counter. With this change, two or more content-filtered subscriptions can be created for the same topic name.

before this fix, we cannot have the content filtered topic on the topic within the same participant, correct? and that is the problem to address with this PR?

fujitatomoya
fujitatomoya approved these changes Jun 1, 2024
View reviewed changes

@fujitatomoya fujitatomoya left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm with green CI.

I would like to ask someone from eProsima to review this.

CC: @MiguelCompany @EduPonz

@MiguelCompany

MiguelCompany commented Jun 3, 2024

Copy link
Copy Markdown
Collaborator

before this fix, we cannot have the content filtered topic on the topic within the same participant, correct? and that is the problem to address with this PR?

Before this fix, we cannot have more than one content filter on the same topic within the same participant. Correct.

I would like to ask someone from eProsima to review this.

We already did, since @Mario-DL works at eProsima. See my approval here

@ahcorde

ahcorde commented Jun 3, 2024

Copy link
Copy Markdown
Contributor
  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ahcorde

ahcorde commented Jun 3, 2024

Copy link
Copy Markdown
Contributor
  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ahcorde
ahcorde merged commit 97edce2 into ros2:rolling Jun 3, 2024
@fujitatomoya

fujitatomoya commented Jun 3, 2024

Copy link
Copy Markdown
Collaborator

@ahcorde thanks for taking care of CI.

@Mario-DL @MiguelCompany how does it sound that we backport this to jazzy, iron and humble? since this is just adding the static local variable in function scope, it is okay to backport and user-experience is better? if okay, i will manage all backports including CI. what do you think?

@MiguelCompany

MiguelCompany commented Jun 3, 2024

Copy link
Copy Markdown
Collaborator

how does it sound that we backport this to jazzy, iron and humble?

@fujitatomoya it sounds great. I think it makes sense to backport

@fujitatomoya

fujitatomoya commented Jun 5, 2024

Copy link
Copy Markdown
Collaborator

@Mergifyio backport iron humble jazzy

@mergify

mergify Bot commented Jun 5, 2024 •
edited
Loading

Copy link
Copy Markdown

backport iron humble jazzy

✅ Backports have been created

Details

mergify Bot pushed a commit that referenced this pull request Jun 5, 2024
@Mario-DL @mergify
Use unique mangled names when creating Content Filter Topics (#762)
98477c1 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
Signed-off-by: Mario-DL <mariodominguez@eprosima.com>
(cherry picked from commit 97edce2)
mergify Bot pushed a commit that referenced this pull request Jun 5, 2024
@Mario-DL @mergify
Use unique mangled names when creating Content Filter Topics (#762)
7967491 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
Signed-off-by: Mario-DL <mariodominguez@eprosima.com>
(cherry picked from commit 97edce2)
This was referenced Jun 5, 2024
Merged
Merged
mergify Bot pushed a commit that referenced this pull request Jun 5, 2024
@Mario-DL @mergify
Use unique mangled names when creating Content Filter Topics (#762)
c12f92d 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
Signed-off-by: Mario-DL <mariodominguez@eprosima.com>
(cherry picked from commit 97edce2)
@mergify mergify Bot mentioned this pull request Jun 5, 2024
Merged
ahcorde pushed a commit that referenced this pull request Jun 6, 2024
@mergify @Mario-DL
Use unique mangled names when creating Content Filter Topics (#762) (#…
58b71ef 
…769)

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
Signed-off-by: Mario-DL <mariodominguez@eprosima.com>
(cherry picked from commit 97edce2)

Co-authored-by: Mario Domínguez López <116071334+Mario-DL@users.noreply.github.com>
fujitatomoya pushed a commit that referenced this pull request Jun 7, 2024
@mergify @Mario-DL
Use unique mangled names when creating Content Filter Topics (#762) (#…
a997f6e 
…767)

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
Signed-off-by: Mario-DL <mariodominguez@eprosima.com>
(cherry picked from commit 97edce2)

Co-authored-by: Mario Domínguez López <116071334+Mario-DL@users.noreply.github.com>
fujitatomoya pushed a commit that referenced this pull request Jun 12, 2024
@mergify @Mario-DL
Use unique mangled names when creating Content Filter Topics (#762) (#…
243cd3d 
…768)

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
Signed-off-by: Mario-DL <mariodominguez@eprosima.com>
(cherry picked from commit 97edce2)

Co-authored-by: Mario Domínguez López <116071334+Mario-DL@users.noreply.github.com>
@fujitatomoya

fujitatomoya commented Jun 12, 2024

Copy link
Copy Markdown
Collaborator

@Mario-DL @MiguelCompany Just FYI, all backports up to humble completed.

@MiguelCompany
MiguelCompany deleted the fix/rolling/unique-cft-names branch June 12, 2024 06:37
@MiguelCompany

MiguelCompany commented Jun 12, 2024

Copy link
Copy Markdown
Collaborator

Just FYI, all backports up to humble completed.

Thank you very much @fujitatomoya !

@Barry-Xu-2018 Barry-Xu-2018 mentioned this pull request Jun 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahcorde ahcorde ahcorde approved these changes
@fujitatomoya fujitatomoya fujitatomoya approved these changes
@gbiggs gbiggs Awaiting requested review from gbiggs
@sloretz sloretz Awaiting requested review from sloretz

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Mario-DL @fujitatomoya @MiguelCompany @ahcorde