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

[19687] Fix updatability of immutable DataWriterQos #3915

Merged
merged 3 commits into from
Oct 10, 2023
Merged

[19687] Fix updatability of immutable DataWriterQos #3915

merged 3 commits into from
Oct 10, 2023

Conversation

MiguelCompany
Copy link
Member

@MiguelCompany MiguelCompany commented Oct 9, 2023 •
edited by Mario-DL
Loading

Description

This PR fixes a regression introduced in #3879.
It adds a regression test and also refactors the method performing the DataWriterQos assignment.
This regression was discovered while reviewing the backports, so it does not need to be automatically backported.
Its merge commit should be cherry-picked in the backports for #3879 instead.

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
  • N/A Any new/modified methods have been properly documented using Doxygen.
  • Changes are ABI compatible.
  • Changes are API compatible.
  • N/A 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.
    Related documentation PR: [19687] Fix documentation on immutable DataWriterQos Fast-DDS-docs#575
  • N/A (see description) Applicable backports have been included in the description.

Reviewer Checklist

  • The PR has a milestone assigned.
  • Check contributor checklist is correct.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

@MiguelCompany MiguelCompany added this to the v2.12.1 milestone Oct 9, 2023
@MiguelCompany MiguelCompany mentioned this pull request Oct 9, 2023
Merged
Mario-DL
Mario-DL approved these changes Oct 9, 2023
View reviewed changes
Copy link
Member

@Mario-DL Mario-DL left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice catch. We were not updating immutable QoS if DataWriter was disabled.
Refactor makes the seq_qos() more readable. I recognize I struggled interpreting the meaning of the old is_default bool.
LGTM with green CI.

This was referenced Oct 9, 2023
Merged
Merged
Merged
@Mario-DL
Copy link
Member

asan failures clearly unrelated with this PR

@MiguelCompany MiguelCompany merged commit f631faa into master Oct 10, 2023
12 of 14 checks passed
@MiguelCompany MiguelCompany deleted the hotfix/19687 branch October 10, 2023 05:54
MiguelCompany added a commit that referenced this pull request Oct 10, 2023
@MiguelCompany
Fix updatability of immutable DataWriterQos (#3915)
1a57342 
* Refs #19687. Add regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #19687. Rename argument.

Signed-off-by: Miguel Company <[email protected]>

* Refs #19687. Refactor on DataWriterImpl::set_qos.

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>
MiguelCompany added a commit that referenced this pull request Oct 10, 2023
@MiguelCompany
Fix updatability of immutable DataWriterQos (#3915)
7db38f4 
* Refs #19687. Add regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #19687. Rename argument.

Signed-off-by: Miguel Company <[email protected]>

* Refs #19687. Refactor on DataWriterImpl::set_qos.

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>
MiguelCompany added a commit that referenced this pull request Oct 10, 2023
@MiguelCompany
Fix updatability of immutable DataWriterQos (#3915)
5d2ff42 
* Refs #19687. Add regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #19687. Rename argument.

Signed-off-by: Miguel Company <[email protected]>

* Refs #19687. Refactor on DataWriterImpl::set_qos.

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>
MiguelCompany added a commit that referenced this pull request Oct 26, 2023
@mergify @cferreiragonz @MiguelCompany
Updatable disable_positive_acks period (#3896)
9e8be04 
* Updatable disable_positive_acks period (#3879)

* Refs #19576: Test Update positive_acks period on RTPS Layer

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19567: Fix Update positive_acks period on RTPS Layer

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19576: Test Update positive_acks period on DDS Layer

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19576: Fix ack_timer and Updatability of positive_acks on DDS Layer

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19576: Uncrustify fix

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19576: Fix linux ci

Signed-off-by: cferreiragonz <[email protected]>

* Ref #19576: DataReaderImpl update and updateAttributes call

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19576: Fix mac-ci

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19576: move if clause

Signed-off-by: cferreiragonz <[email protected]>

---------

Signed-off-by: cferreiragonz <[email protected]>
(cherry picked from commit b84825a)

* Fix updatability of immutable DataWriterQos (#3915)

* Refs #19687. Add regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #19687. Rename argument.

Signed-off-by: Miguel Company <[email protected]>

* Refs #19687. Refactor on DataWriterImpl::set_qos.

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>

* Fix build after rebase

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>
Co-authored-by: Carlos Ferreira <[email protected]>
Co-authored-by: Miguel Company <[email protected]>
MiguelCompany added a commit that referenced this pull request Oct 26, 2023
@mergify @cferreiragonz @MiguelCompany
Updatable disable_positive_acks period (#3895)
bd34703 
* Updatable disable_positive_acks period (#3879)

* Refs #19576: Test Update positive_acks period on RTPS Layer

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19567: Fix Update positive_acks period on RTPS Layer

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19576: Test Update positive_acks period on DDS Layer

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19576: Fix ack_timer and Updatability of positive_acks on DDS Layer

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19576: Uncrustify fix

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19576: Fix linux ci

Signed-off-by: cferreiragonz <[email protected]>

* Ref #19576: DataReaderImpl update and updateAttributes call

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19576: Fix mac-ci

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19576: move if clause

Signed-off-by: cferreiragonz <[email protected]>

---------

Signed-off-by: cferreiragonz <[email protected]>
(cherry picked from commit b84825a)

* Fix updatability of immutable DataWriterQos (#3915)

* Refs #19687. Add regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #19687. Rename argument.

Signed-off-by: Miguel Company <[email protected]>

* Refs #19687. Refactor on DataWriterImpl::set_qos.

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>

* Fix build after rebase

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>
Co-authored-by: Carlos Ferreira <[email protected]>
Co-authored-by: Miguel Company <[email protected]>
MiguelCompany added a commit that referenced this pull request Oct 30, 2023
@mergify @cferreiragonz @MiguelCompany
Updatable disable_positive_acks period (#3897)
1369e67 
* Updatable disable_positive_acks period (#3879)

* Refs #19576: Test Update positive_acks period on RTPS Layer

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19567: Fix Update positive_acks period on RTPS Layer

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19576: Test Update positive_acks period on DDS Layer

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19576: Fix ack_timer and Updatability of positive_acks on DDS Layer

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19576: Uncrustify fix

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19576: Fix linux ci

Signed-off-by: cferreiragonz <[email protected]>

* Ref #19576: DataReaderImpl update and updateAttributes call

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19576: Fix mac-ci

Signed-off-by: cferreiragonz <[email protected]>

* Refs #19576: move if clause

Signed-off-by: cferreiragonz <[email protected]>

---------

Signed-off-by: cferreiragonz <[email protected]>
(cherry picked from commit b84825a)

# Conflicts:
#	test/blackbox/api/dds-pim/PubSubWriter.hpp

* Fix Mergify conflict

* Fix updatability of immutable DataWriterQos (#3915)

* Refs #19687. Add regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #19687. Rename argument.

Signed-off-by: Miguel Company <[email protected]>

* Refs #19687. Refactor on DataWriterImpl::set_qos.

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>

* Fixed log macro.

Signed-off-by: Miguel Company <[email protected]>

* Fix build error.

Signed-off-by: Miguel Company <[email protected]>

* Remove unused header

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>
Co-authored-by: Carlos Ferreira <[email protected]>
Co-authored-by: Carlos Ferreira <[email protected]>
Co-authored-by: Miguel Company <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mario-DL Mario-DL Mario-DL approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.12.1
Development

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