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

[21745] Add QoS getters from raw XML #5256

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

[21745] Add QoS getters from raw XML #5256

wants to merge 15 commits into from

Conversation

juanlofer-eprosima
Copy link
Contributor

@juanlofer-eprosima juanlofer-eprosima commented Sep 24, 2024 •
edited by JesusPoderoso
Loading

Description

This PR extends public API with methods meant to fill QoS structures given a raw XML string containing pertinent profile configurations. All new methods accept an optional profile_name argument with the one to be retrieved. If this argument is empty, the first encountered profile of the expected kind (participant, publisher, etc.) is parsed. Additionally, profiles represented in the raw XML string are more flexible in their structure (e.g. profile_name is not required, and they can be injected at multiple levels (not just under profiles tag)).

This API extension provides backwards compatibility after having made XMLParser private (done in Fast-DDS 3 release).

Merge after:

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • 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.
  • N/A Any new configuration API has an equivalent XML API (with the corresponding XSD extension)
  • Changes are backport compatible: they do NOT break ABI nor change library core behavior.
  • Changes are API compatible.
  • New feature has been added to the versions.md file (if applicable).
  • New feature has been documented/Current behavior is correctly described in the documentation.
  • 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.

juanlofer-eprosima
juanlofer-eprosima commented Sep 24, 2024
View reviewed changes
src/cpp/xmlparser/XMLProfileManager.cpp
Comment on lines +52 to +57
template<typename T>
struct AttributesTraits;

template<>
struct AttributesTraits<ParticipantAttributes>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move these to XMLProfileManager header? Or a dedicated header?

@juanlofer-eprosima juanlofer-eprosima changed the title Add QoS getters from raw XML [21745] Add QoS getters from raw XML Sep 24, 2024
@juanlofer-eprosima juanlofer-eprosima force-pushed the feature/get-qos-from-xml branch 2 times, most recently from 4ae3aa2 to 4f0b453 Compare September 24, 2024 08:57
@juanlofer-eprosima juanlofer-eprosima force-pushed the feature/get-qos-from-xml branch 2 times, most recently from 4bfe8b3 to 2c61624 Compare October 2, 2024 13:15
@juanlofer-eprosima juanlofer-eprosima marked this pull request as ready for review October 2, 2024 13:30
@juanlofer-eprosima
Refs #21745. Compare FlowControllerDescriptor content instead of poin…
28d66fe 
…ter address

Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
@juanlofer-eprosima
Refs #21745. Update qos comparison method in tests
04fa7d2 
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
@juanlofer-eprosima juanlofer-eprosima requested review from richiprosima and removed request for richiprosima October 3, 2024 07:50
@github-actions github-actions bot added the ci-pending PR which CI is running label Oct 3, 2024
@juanlofer-eprosima
Refs #21745. Make lambda a reusable public method
8a3fd08 
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
@juanlofer-eprosima
Refs #21745. Add QoS getters from raw XML
0e27a38 
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
@juanlofer-eprosima
Refs #21745. Add from_profile overloads with extra info
18c43e6 
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
@juanlofer-eprosima
Refs #21745. Add get_participant_extended_qos_from_xml
7aba4f6 
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
@juanlofer-eprosima
Refs #21745. Fix typo
3a2e97d 
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
@juanlofer-eprosima
Refs #21745. Export DomainParticipant::has_active_entities
7cb0b48 
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
@juanlofer-eprosima
Refs #21745. Change skip condition in fill_attributes_from_xml when p…
4127259 
…rofile requested

Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
@juanlofer-eprosima
Refs #21745. Add docstrings for new methods
63d2a89 
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
@juanlofer-eprosima
Refs #21745. Refactor fill_attributes_from_xml to parse recursively
20ba8c9 
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
@juanlofer-eprosima
Refs #21745. Add more overloads and remove redundant implementations
b1b4328 
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
@juanlofer-eprosima
Refs #21745. Add unittests
fc576ea 
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
@juanlofer-eprosima
Refs #21745. Fix docstring
9f5ab64 
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
@juanlofer-eprosima
Refs #21745. Perform full DomainParticipantQos comparison in tests
fb0a418 
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
@juanlofer-eprosima juanlofer-eprosima requested review from richiprosima and removed request for richiprosima October 3, 2024 08:45
@JesusPoderoso JesusPoderoso added this to the v3.1.0 - Frozen ❄️ milestone Oct 4, 2024
richiware
richiware requested changes Oct 4, 2024
View reviewed changes
Copy link
Member

@richiware richiware left a comment

Choose a reason for hiding this comment

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

Partial review

include/fastdds/dds/domain/DomainParticipant.hpp
* @param profile_name Publisher profile name. Empty by default (first one found).
* @return RETCODE_OK on success. RETCODE_BAD_PARAMETER otherwise.
*/
FASTDDS_EXPORTED_API ReturnCode_t get_publisher_qos_from_xml(
Copy link
Member

Choose a reason for hiding this comment

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

We decided to have at least two separate functions.

An overload with profile_name argument. This function will expects a complete Fast DDS XML and will retrieve the profile specified. In case profile_name is empty, might return the default one (this can be done in another get_default_... function. If the profile is not found, return error.

An overload function without profile_name argument. This XML might not follow the Fast DDS XML structure and will retrieve the first profile found.

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

@richiware richiware richiware requested changes

Requested changes must be addressed to merge this pull request.

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

Successfully merging this pull request may close these issues.
3 participants
@juanlofer-eprosima @richiware @JesusPoderoso