Skip to content

Use unique mangled names when creating Content Filter Topics#762

Merged
ahcorde merged 2 commits intoros2: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 intoros2:rollingfrom
eProsima:fix/rolling/unique-cft-names

Conversation

@Mario-DL
Copy link
Copy Markdown
Contributor

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

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
Copy link
Copy Markdown
Contributor

@ahcorde ahcorde left a comment

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

@Mario-DL
Rev changes
92fadee 
Signed-off-by: Mario-DL <mariodominguez@eprosima.com>
@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented May 31, 2024

@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
Copy link
Copy Markdown
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.

lgtm with green CI.

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

CC: @MiguelCompany @EduPonz

@MiguelCompany
Copy link
Copy Markdown
Collaborator

MiguelCompany commented Jun 3, 2024

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
Copy link
Copy Markdown
Contributor

ahcorde commented Jun 3, 2024

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

@ahcorde
Copy link
Copy Markdown
Contributor

ahcorde commented Jun 3, 2024

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

@ahcorde ahcorde merged commit 97edce2 into ros2:rolling Jun 3, 2024
@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Jun 3, 2024

@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
Copy link
Copy Markdown
Collaborator

MiguelCompany commented Jun 3, 2024

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
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Jun 5, 2024

@Mergifyio backport iron humble jazzy

@mergify
Copy link
Copy Markdown

mergify bot commented Jun 5, 2024 •
edited
Loading

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
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Jun 12, 2024

@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
Copy link
Copy Markdown
Collaborator

MiguelCompany commented Jun 12, 2024

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