Skip to content

[22875] Make *ProxyData inherit from *BuiltinTopicData and refactor it #5699

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
Merged
merged 24 commits into from
Mar 18, 2025
Merged

[22875] Make *ProxyData inherit from *BuiltinTopicData and refactor it #5699

merged 24 commits into from
Mar 18, 2025

Conversation

Mario-DL
Copy link
Member

@Mario-DL Mario-DL commented Mar 10, 2025 •
edited by MiguelCompany
Loading

Description

This PR

  • Makes Participant/Writer/ReaderProxyData inherit from Participant/Publication/SubscriptionBuiltinTopicData
  • Removes the from_proxy/builtin/to_proxy/builtin helper converters.
  • As a consequence, it avoids the intermediate copy when on_*_discovery callbacks are invoked.
  • Refactors the ProxyData interfaces to be `snake case.
  • Adds discourage marks in the WriterQos and ReaderQos classes in favor of PublicationBuiltinTopicData and SubscriptionBuiltinTopicData.
  • It removes the time_based_filter from writer's proxy data serialization and the durability_service from the reader's proxy data serialization.

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • N/A Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
  • Any new/modified methods have been properly documented using Doxygen.
  • Any new configuration API has an equivalent XML API (with the corresponding XSD extension)
  • NO Changes are backport compatible: they do NOT break ABI nor change library core behavior.
  • Changes are API compatible.
  • N/A New feature has been added to the versions.md file (if applicable).
  • N/A New feature has been documented/Current behavior is correctly described in the documentation.
  • N/A Applicable backports have been included in the description.

Reviewer Checklist

  • The PR has a milestone assigned.
  • The title and description correctly express the PR's purpose.
  • Check contributor checklist is correct.
  • If this is a critical bug fix, backports to the critical-only supported branches have been requested.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

@Mario-DL Mario-DL added this to the v3.2.0 milestone Mar 10, 2025
@Mario-DL Mario-DL force-pushed the feature/22875 branch 5 times, most recently from 11b2761 to 76bb4dd Compare March 12, 2025 15:26
@Mario-DL Mario-DL marked this pull request as ready for review March 12, 2025 15:45
@Mario-DL Mario-DL requested review from richiprosima and removed request for richiprosima March 12, 2025 15:50
@github-actions github-actions bot added the ci-pending PR which CI is running label Mar 12, 2025
@Mario-DL Mario-DL requested review from MiguelCompany and removed request for MiguelCompany March 13, 2025 11:06
@Mario-DL Mario-DL requested review from MiguelCompany and removed request for MiguelCompany March 13, 2025 14:37
Mario-DL added 12 commits March 13, 2025 16:50
@Mario-DL
Refs #22875: Add custom constructor in ParticipantBuiltinTopicData
305b862 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: Remove Participant's from_proxy_to_builtin() conversions…
fbf3dbf 
… and rename builtin topic key helpers

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: Make ParticipantProxyData inherit from ParticipantBuilti…
f068dea 
…nTopicData and refactor it

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: Pass the ParticipantProxyData object in listeners
44c2be3 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: Refactor and rename the rest of occurences as a result o…
050d0c3 
…f the changes in ParticipantProxyData

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: PublicationBuiltinTopicData: Add methods and constructor
90ad99c 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: Make WriterProxyData inherit from PublicationBuiltinTopi…
ad26d98 
…cData and refactor it

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: Remove Publication's from_proxy_to_builtin() conversions
e303e86 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: Pass the WriterProxyData in listeners
c18bdde 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: Refactor and rename the rest of occurences as a result o…
492cc43 
…f the changes in WriterProxyData

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: SubscriptionBuiltinTopicData: Add methods and constructors
d7dd71b 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: Make ReaderProxyData inherit from SubscriptionBuiltinTop…
de04f32 
…icData and refactor it

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
Mario-DL added 10 commits March 13, 2025 16:50
@Mario-DL
Refs #22875: Pass the ReaderProxyData in listeners
8b9149e 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: Remove ProxyDataConverters
571745d 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: Refactor and rename the rest of occurrences as a result …
0c6322c 
…of the changes in ReaderProxyData

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: Rename read/writeFrom/ToCDRmessage to snake case in Prox…
559e011 
…yDatas

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: Add TODO_BEFORE the next major release and discourage ma…
591cd52 
…rks in the use of Writer/ReaderQoS

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: Linter
3087dbc 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: Remove redundant m_type_information member from Writer/R…
deaf2ed 
…eaderProxyData

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: Linter v2
502d28b 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: Fix windows warnings
50710ce 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL
Refs #22875: Fix NO SECURITY build and DS tests
389e9f0 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL Mario-DL requested review from MiguelCompany and removed request for MiguelCompany March 13, 2025 15:52
MiguelCompany
MiguelCompany requested changes Mar 17, 2025
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

Revision for include and src. Tests pending.

MiguelCompany
MiguelCompany reviewed Mar 17, 2025
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

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

Changes in tests LGTM

@Mario-DL
Refs #22875: Apply Miguel's review
0e13180 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL Mario-DL requested review from MiguelCompany and removed request for MiguelCompany March 17, 2025 14:21
MiguelCompany
MiguelCompany requested changes Mar 17, 2025
View reviewed changes
@Mario-DL
Refs #22875: Apply second review
9bd89b8 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@Mario-DL Mario-DL requested review from MiguelCompany and removed request for MiguelCompany March 17, 2025 14:51
MiguelCompany
MiguelCompany approved these changes Mar 17, 2025
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany 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

@MiguelCompany
Copy link
Member

All failures are known

@MiguelCompany MiguelCompany merged commit ffc9986 into master Mar 18, 2025
15 of 17 checks passed
@MiguelCompany MiguelCompany deleted the feature/22875 branch March 18, 2025 06:50
@cferreiragonz cferreiragonz mentioned this pull request Apr 16, 2025
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MiguelCompany MiguelCompany MiguelCompany approved these changes

Assignees
No one assigned
Labels
ci-pending PR which CI is running
Projects
None yet
Milestone
v3.2.0 - Frozen ❄️
Development

Successfully merging this pull request may close these issues.
2 participants
@Mario-DL @MiguelCompany