Skip to content

[23603] Avoid sending duplicated ACKs in DataSharing (backport #5986)#6004

Merged
rsanchez15 merged 7 commits into2.14.xfrom
mergify/bp/2.14.x/pr-5986
Oct 23, 2025
Merged

[23603] Avoid sending duplicated ACKs in DataSharing (backport #5986)#6004
rsanchez15 merged 7 commits into2.14.xfrom
mergify/bp/2.14.x/pr-5986

Conversation

@mergify
Copy link
Copy Markdown
Contributor

@mergify mergify bot commented Sep 3, 2025

Description

This PR improves DataSharing efficiency by removing an extra ACK that was being sent when StatefulReader::end_sample_access_nts and DataReaderHistory::remove_change_sub were being called consecutively.

@Mergifyio backport 3.3.x 3.2.x 2.14.x

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.

  • 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.

  • 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.

  • 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.
This is an automatic backport of pull request #5986 done by [Mergify](https://mergify.com).

@mergify
Copy link
Copy Markdown
Contributor Author

mergify bot commented Sep 3, 2025

Cherry-pick of 8421fb0 has failed:

On branch mergify/bp/2.14.x/pr-5986
Your branch is up to date with 'origin/2.14.x'.

You are currently cherry-picking commit 8421fb02.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	both modified:   include/fastdds/rtps/reader/StatefulReader.h
	both modified:   include/fastdds/rtps/reader/StatelessReader.h
	both modified:   src/cpp/fastdds/subscriber/DataReaderImpl/ReadTakeCommand.hpp
	deleted by us:   src/cpp/rtps/reader/BaseReader.hpp
	both modified:   src/cpp/rtps/reader/StatefulReader.cpp
	both modified:   src/cpp/rtps/reader/StatelessReader.cpp
	deleted by us:   test/mock/rtps/RTPSReader/rtps/reader/BaseReader.hpp

no changes added to commit (use "git add" and/or "git commit -a")

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@mergify mergify bot added the conflicts Backport PR wich git cherry pick failed label Sep 3, 2025
@mergify mergify bot mentioned this pull request Sep 3, 2025
Merged
12 tasks
@MiguelCompany MiguelCompany added this to the v2.14.6 milestone Sep 3, 2025
@emiliocuestaf emiliocuestaf self-requested a review September 15, 2025 14:12
@emiliocuestaf emiliocuestaf added ci-pending PR which CI is running and removed conflicts Backport PR wich git cherry pick failed labels Oct 20, 2025
@emiliocuestaf emiliocuestaf removed their request for review October 20, 2025 10:28
@emiliocuestaf emiliocuestaf removed the ci-pending PR which CI is running label Oct 20, 2025
@github-actions github-actions bot added the ci-pending PR which CI is running label Oct 20, 2025
@mergify
Copy link
Copy Markdown
Contributor Author

mergify bot commented Oct 20, 2025 •
edited
Loading

🧪 CI Insights

Here's what we observed from your CI run for 6a0918a.

❌ Job Failures

Pipeline Job Health on 2.14.x Retries 🔍 CI Insights 📄 Logs
Fast DDS Linters CI uncrustify Unknown 0 View View
Fast-DDS MacOS CI mac-ci / fastdds_test () Unknown 0 View View

cferreiragonz
cferreiragonz requested changes Oct 21, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@cferreiragonz cferreiragonz left a comment

Choose a reason for hiding this comment

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

Small NIT

cferreiragonz
cferreiragonz requested changes Oct 21, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@cferreiragonz cferreiragonz left a comment

Choose a reason for hiding this comment

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

Adding last change so you can push it from here. Ask mergify to rebase after pushing it as uncrustify is failing due to changes on generated on code on 2.14.x

cferreiragonz
cferreiragonz previously approved these changes Oct 21, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@cferreiragonz cferreiragonz left a comment

Choose a reason for hiding this comment

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

  • LGTM after rebasing when #6084 is merged

@MiguelCompany
Copy link
Copy Markdown
Member

MiguelCompany commented Oct 22, 2025

@Mergifyio rebase

cferreiragonz and others added 5 commits October 22, 2025 06:26
@cferreiragonz @MiguelCompany
Avoid sending duplicated ACKs in DataSharing (#5986)
82ee07d 
* Refs #23603: Avoid duplicate ACK in datasharing

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

* Refs #23603: Doxygen

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

* Refs #23603: Mock method

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

---------

Signed-off-by: cferreiragonz <carlosferreira@eprosima.com>
(cherry picked from commit 8421fb0)

# Conflicts:
#	include/fastdds/rtps/reader/StatefulReader.h
#	include/fastdds/rtps/reader/StatelessReader.h
#	src/cpp/fastdds/subscriber/DataReaderImpl/ReadTakeCommand.hpp
#	src/cpp/rtps/reader/BaseReader.hpp
#	src/cpp/rtps/reader/StatefulReader.cpp
#	src/cpp/rtps/reader/StatelessReader.cpp
#	test/mock/rtps/RTPSReader/rtps/reader/BaseReader.hpp
@emiliocuestaf @MiguelCompany
Fix backport issues
54b9ebc 
Signed-off-by: Emilio Cuesta <emiliocuesta@eprosima.com>
@emiliocuestaf @cferreiragonz @MiguelCompany
Update include/fastdds/rtps/reader/StatelessReader.h
01fe835 
Co-authored-by: Carlos Ferreira González <carlosferreira@eprosima.com>
Signed-off-by: Emilio Cuesta Fernandez <emiliocuesta@eprosima.com>
@emiliocuestaf @cferreiragonz @MiguelCompany
Update src/cpp/rtps/reader/StatelessReader.cpp
60ffb2e 
Co-authored-by: Carlos Ferreira González <carlosferreira@eprosima.com>
Signed-off-by: Emilio Cuesta Fernandez <emiliocuesta@eprosima.com>
@emiliocuestaf @cferreiragonz @MiguelCompany
Update include/fastdds/rtps/reader/StatefulReader.h
15f882b 
Co-authored-by: Carlos Ferreira González <carlosferreira@eprosima.com>
Signed-off-by: Emilio Cuesta Fernandez <emiliocuesta@eprosima.com>
@mergify
Copy link
Copy Markdown
Contributor Author

mergify bot commented Oct 22, 2025

rebase

✅ Branch has been successfully rebased

@MiguelCompany MiguelCompany force-pushed the mergify/bp/2.14.x/pr-5986 branch from 133b177 to 15f882b Compare October 22, 2025 06:26
MiguelCompany
MiguelCompany requested changes Oct 22, 2025
View reviewed changes
@emiliocuestaf @MiguelCompany
Update include/fastdds/rtps/reader/StatefulReader.h
2ff5a68 
Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
Signed-off-by: Emilio Cuesta Fernandez <emiliocuesta@eprosima.com>
@emiliocuestaf @MiguelCompany
Update include/fastdds/rtps/reader/StatelessReader.h
6a0918a 
Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
Signed-off-by: Emilio Cuesta Fernandez <emiliocuesta@eprosima.com>
@MiguelCompany MiguelCompany self-requested a review October 22, 2025 09:52
MiguelCompany
MiguelCompany approved these changes Oct 22, 2025
View reviewed changes
Copy link
Copy Markdown
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

@rsanchez15 rsanchez15 merged commit bbcf0a8 into 2.14.x Oct 23, 2025
19 of 22 checks passed
@rsanchez15 rsanchez15 deleted the mergify/bp/2.14.x/pr-5986 branch October 23, 2025 05:35
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

@richiprosima richiprosima Awaiting requested review from richiprosima

@cferreiragonz cferreiragonz Awaiting requested review from cferreiragonz

Assignees

No one assigned

Labels

ci-pending PR which CI is running

Projects

None yet

Milestone

v2.14.6 - Frozen ❄️

Development

Successfully merging this pull request may close these issues.

4 participants

@MiguelCompany @cferreiragonz @emiliocuestaf @rsanchez15