[19576] Updatable disable_positive_acks period #3879

cferreiragonz · 2023-09-26T10:35:36Z

Description

@Mergifyio backport 2.11.x 2.10.x 2.6.x

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.
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: Refs #19576: Docs Updatability of positive_acks period Fast-DDS-docs#559 (PR)
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.

cferreiragonz · 2023-09-26T10:40:09Z

@Mergifyio backport 2.11.x 2.10.x 2.6.x

mergify · 2023-09-26T10:40:20Z

backport 2.11.x 2.10.x 2.6.x

✅ Backports have been created

#3895 [19576] Updatable disable_positive_acks period (backport #3879) has been created for branch 2.11.x
#3896 [19576] Updatable disable_positive_acks period (backport #3879) has been created for branch 2.10.x
#3897 [19576] Updatable disable_positive_acks period (backport #3879) has been created for branch 2.6.x but encountered conflicts

Mario-DL

Really interesting PR fixing the behavior of disable_positive_acks, good job !
Leaving some comments below.
Also, we need to keep an eye on the asan ci results. It seems that some of the fails may be related with the PR

test/blackbox/common/DDSBlackboxTestsAckPositive.cpp

src/cpp/rtps/writer/StatefulWriter.cpp

test/blackbox/common/RTPSBlackboxTestsPositiveAck.cpp

test/blackbox/common/DDSBlackboxTestsAckPositive.cpp

test/blackbox/common/RTPSBlackboxTestsPositiveAck.cpp

src/cpp/rtps/writer/StatefulWriter.cpp

test/blackbox/common/DDSBlackboxTestsAckPositive.cpp

test/blackbox/common/RTPSBlackboxTestsPositiveAck.cpp

Mario-DL · 2023-09-27T06:20:09Z

@richiprosima please test this

cferreiragonz · 2023-09-27T13:23:51Z

@richiprosima please test this

Mario-DL

Nice modifications to overcome the previous test failures !
Couple of comments left:

An improvement suggestion.
I think we forgot modifying the DataReaderImpl https://github.com/eProsima/Fast-DDS/blob/39da2481c9f5bdd5b94c7479e4b59d6e6a7af50a/src/cpp/fastdds/subscriber/DataReaderImpl.cpp#L1670 for checking if user modifies the disable_positive_acks at runtime (like in the writer)

Note: Before applying the changes, please, rebase this branch on top of master

src/cpp/fastdds/publisher/DataWriterImpl.cpp

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

…ayer Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

Mario-DL · 2023-09-28T14:28:35Z

@richiprosima please test this

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

cferreiragonz · 2023-09-29T06:47:21Z

@richiprosima please test mac

Mario-DL

LGTM with green CI

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

Mario-DL

LGTM with green CI !

Mario-DL · 2023-09-29T10:09:42Z

@richiprosima please test this

* Refs #19576: Test Update positive_acks period on RTPS Layer Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19567: Fix Update positive_acks period on RTPS Layer Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Test Update positive_acks period on DDS Layer Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Fix ack_timer and Updatability of positive_acks on DDS Layer Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Uncrustify fix Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Fix linux ci Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Ref #19576: DataReaderImpl update and updateAttributes call Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Fix mac-ci Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: move if clause Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> --------- Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> (cherry picked from commit b84825a)

* Refs #19576: Test Update positive_acks period on RTPS Layer Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19567: Fix Update positive_acks period on RTPS Layer Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Test Update positive_acks period on DDS Layer Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Fix ack_timer and Updatability of positive_acks on DDS Layer Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Uncrustify fix Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Fix linux ci Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Ref #19576: DataReaderImpl update and updateAttributes call Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Fix mac-ci Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: move if clause Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> --------- Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> (cherry picked from commit b84825a) # Conflicts: # test/blackbox/api/dds-pim/PubSubWriter.hpp

* Updatable disable_positive_acks period (#3879) * Refs #19576: Test Update positive_acks period on RTPS Layer Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19567: Fix Update positive_acks period on RTPS Layer Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Test Update positive_acks period on DDS Layer Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Fix ack_timer and Updatability of positive_acks on DDS Layer Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Uncrustify fix Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Fix linux ci Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Ref #19576: DataReaderImpl update and updateAttributes call Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Fix mac-ci Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: move if clause Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> --------- Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> (cherry picked from commit b84825a) * Fix updatability of immutable DataWriterQos (#3915) * Refs #19687. Add regression test. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #19687. Rename argument. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #19687. Refactor on DataWriterImpl::set_qos. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> --------- Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Fix build after rebase Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> --------- Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> Co-authored-by: Carlos Ferreira <carloos.499@gmail.com> Co-authored-by: Miguel Company <miguelcompany@eprosima.com>

* Updatable disable_positive_acks period (#3879) * Refs #19576: Test Update positive_acks period on RTPS Layer Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19567: Fix Update positive_acks period on RTPS Layer Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Test Update positive_acks period on DDS Layer Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Fix ack_timer and Updatability of positive_acks on DDS Layer Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Uncrustify fix Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Fix linux ci Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Ref #19576: DataReaderImpl update and updateAttributes call Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: Fix mac-ci Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> * Refs #19576: move if clause Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> --------- Signed-off-by: cferreiragonz <carlosferreira@eprosima.com> (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 <MiguelCompany@eprosima.com> * Refs #19687. Rename argument. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Refs #19687. Refactor on DataWriterImpl::set_qos. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> --------- Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Fixed log macro. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Fix build error. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * Remove unused header Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> --------- Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> Co-authored-by: Carlos Ferreira <carloos.499@gmail.com> Co-authored-by: Carlos Ferreira <carlosferreira@eprosima.com> Co-authored-by: Miguel Company <miguelcompany@eprosima.com>

cferreiragonz added this to the v2.12.1 milestone Sep 26, 2023

cferreiragonz added needs-review PR that is ready to be reviewed ci-pending PR which CI is running labels Sep 26, 2023

cferreiragonz self-assigned this Sep 26, 2023

cferreiragonz requested a review from Mario-DL September 26, 2023 10:42

Mario-DL requested changes Sep 26, 2023

View reviewed changes

Mario-DL removed the needs-review PR that is ready to be reviewed label Sep 27, 2023

cferreiragonz requested a review from Mario-DL September 27, 2023 13:24

Mario-DL requested changes Sep 28, 2023

View reviewed changes

src/cpp/fastdds/publisher/DataWriterImpl.cpp Outdated Show resolved Hide resolved

cferreiragonz added 6 commits September 28, 2023 12:30

Refs #19576: Test Update positive_acks period on RTPS Layer

f2b6211

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

Refs #19567: Fix Update positive_acks period on RTPS Layer

4bd3d25

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

Refs #19576: Test Update positive_acks period on DDS Layer

56e1b57

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

Refs #19576: Fix ack_timer and Updatability of positive_acks on DDS L…

be1a9e2

…ayer Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

Refs #19576: Uncrustify fix

6ef004e

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

Refs #19576: Fix linux ci

970ee23

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

cferreiragonz force-pushed the bugfix/updatability_positive_acks branch from 39da248 to 970ee23 Compare September 28, 2023 10:31

Ref #19576: DataReaderImpl update and updateAttributes call

5b90659

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

cferreiragonz force-pushed the bugfix/updatability_positive_acks branch from 0fe925c to 5b90659 Compare September 28, 2023 14:25

Refs #19576: Fix mac-ci

b2a81b0

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

Mario-DL previously approved these changes Sep 29, 2023

View reviewed changes

Refs #19576: move if clause

29876c8

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>

cferreiragonz dismissed Mario-DL’s stale review via 29876c8 September 29, 2023 10:07

cferreiragonz force-pushed the bugfix/updatability_positive_acks branch from c5e614b to 29876c8 Compare September 29, 2023 10:07

Mario-DL approved these changes Sep 29, 2023

View reviewed changes

Mario-DL added ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. and removed ci-pending PR which CI is running labels Oct 2, 2023

EduPonz merged commit b84825a into master Oct 2, 2023

EduPonz deleted the bugfix/updatability_positive_acks branch October 2, 2023 05:56

mergify bot mentioned this pull request Oct 2, 2023

[19576] Updatable disable_positive_acks period (backport #3879) #3895

Merged

mergify bot mentioned this pull request Oct 2, 2023

[19576] Updatable disable_positive_acks period (backport #3879) #3896

Merged

mergify bot mentioned this pull request Oct 2, 2023

[19576] Updatable disable_positive_acks period (backport #3879) #3897

Merged

MiguelCompany mentioned this pull request Oct 9, 2023

[19687] Fix updatability of immutable DataWriterQos #3915

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[19576] Updatable disable_positive_acks period #3879

[19576] Updatable disable_positive_acks period #3879

cferreiragonz commented Sep 26, 2023 •

edited by Mario-DL

Loading

cferreiragonz commented Sep 26, 2023

mergify bot commented Sep 26, 2023 •

edited

Loading

Mario-DL left a comment

Mario-DL commented Sep 27, 2023

cferreiragonz commented Sep 27, 2023

Mario-DL left a comment

Mario-DL commented Sep 28, 2023

cferreiragonz commented Sep 29, 2023

Mario-DL left a comment

Mario-DL left a comment

Mario-DL commented Sep 29, 2023

[19576] Updatable disable_positive_acks period #3879

[19576] Updatable disable_positive_acks period #3879

Conversation

cferreiragonz commented Sep 26, 2023 • edited by Mario-DL Loading

Description

Contributor Checklist

Reviewer Checklist

cferreiragonz commented Sep 26, 2023

mergify bot commented Sep 26, 2023 • edited Loading

✅ Backports have been created

Mario-DL left a comment

Choose a reason for hiding this comment

Mario-DL commented Sep 27, 2023

cferreiragonz commented Sep 27, 2023

Mario-DL left a comment

Choose a reason for hiding this comment

Mario-DL commented Sep 28, 2023

cferreiragonz commented Sep 29, 2023

Mario-DL left a comment

Choose a reason for hiding this comment

Mario-DL left a comment

Choose a reason for hiding this comment

Mario-DL commented Sep 29, 2023

cferreiragonz commented Sep 26, 2023 •

edited by Mario-DL

Loading

mergify bot commented Sep 26, 2023 •

edited

Loading