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

Fix StatelessWriter locators filtering [18950] #3655

Merged
merged 11 commits into from
Jul 11, 2023
Merged

Fix StatelessWriter locators filtering [18950] #3655

merged 11 commits into from
Jul 11, 2023

Conversation

juanlofer-eprosima
Copy link
Contributor

@juanlofer-eprosima juanlofer-eprosima commented Jul 5, 2023 •
edited by MiguelCompany
Loading

Description

The filtering of UDP locators when SHM is available used to be performed when parsing received data Ps (ParticipantProxyData::readFromCDRMessage). #3079 moved this filtering forward to the EDP phase, conserving the UDP locator until ExternalLocatorsProcessor::filter_remote_locators is called in matched_reader_add. This filtering function requires the writer to have their own UDP and SHM locators available (in m_att.external_unicast_locators), so it can determine the reader's UDP one can be discarded. However, this requirement is only fulfilled by StatefulWriters as this attribute is set in writer creation at RTPSParticipantImpl::createAndAssociateReceiverswithEndpoint, currently only invoked for reliable writers.
This PR aims to fix the issue by encapsulating endpoints external locators initialization in a method, to be called when creating all writers irrespective of their reliability kind.

@Mergifyio backport 2.10.x 2.9.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).
  • 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.
  • Check contributor checklist is correct.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

JesusPoderoso and others added 5 commits July 5, 2023 15:38
@JesusPoderoso @juanlofer-eprosima
Refs #18950: Introduce regression test
81d2679 
Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
@JesusPoderoso @juanlofer-eprosima
Refs #18950: Make test build
c381778 
Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
@juanlofer-eprosima
Finalize test
e5efa0a 
Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>
@juanlofer-eprosima
Add fix
61dd20d 
Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>
@juanlofer-eprosima
Uncrustify
fbdbd2c 
Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>
@juanlofer-eprosima juanlofer-eprosima force-pushed the hotfix/best_effort_writer_udp_traffic branch from a3539d3 to fbdbd2c Compare July 5, 2023 13:48
@juanlofer-eprosima juanlofer-eprosima changed the title Fix StatelessWriter locators filtering Fix StatelessWriter locators filtering [18950] Jul 5, 2023
MiguelCompany
MiguelCompany requested changes Jul 5, 2023
View reviewed changes
src/cpp/rtps/participant/RTPSParticipantImpl.cpp Show resolved Hide resolved
src/cpp/rtps/participant/RTPSParticipantImpl.cpp Outdated Show resolved Hide resolved
src/cpp/rtps/participant/RTPSParticipantImpl.cpp Outdated Show resolved Hide resolved
src/cpp/rtps/transport/test_UDPv4Transport.cpp Outdated Show resolved Hide resolved
test/blackbox/common/DDSBlackboxTestsTransportSHMUDP.cpp Outdated Show resolved Hide resolved
test/blackbox/common/DDSBlackboxTestsTransportSHMUDP.cpp Outdated Show resolved Hide resolved
test/blackbox/common/DDSBlackboxTestsTransportSHMUDP.cpp Outdated Show resolved Hide resolved
@EduPonz EduPonz added this to the v2.11.1 milestone Jul 6, 2023
@juanlofer-eprosima
Apply suggestions
9f245e5 
Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>
@juanlofer-eprosima
Separate parametrized tests
1ad167e 
Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>
MiguelCompany
MiguelCompany previously approved these changes Jul 6, 2023
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 MiguelCompany added the ci-pending PR which CI is running label Jul 6, 2023
@juanlofer-eprosima
Issue test compilation warning
dbb0d65 
Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>
MiguelCompany
MiguelCompany previously approved these changes Jul 7, 2023
View reviewed changes
@juanlofer-eprosima
Make messages_sent an object attribute
9f47201 
Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>
@juanlofer-eprosima
Get rid of messages_sent and use object's locator_filter
2950f42 
Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>
MiguelCompany
MiguelCompany requested changes Jul 10, 2023
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.

Just one nit

test/blackbox/common/DDSBlackboxTestsTransportSHMUDP.cpp Outdated Show resolved Hide resolved
@juanlofer-eprosima
Apply suggestion
8bc81f7 
Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>
MiguelCompany
MiguelCompany approved these changes Jul 10, 2023
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

@richiprosima Please test this

@JesusPoderoso
Copy link
Contributor

@richiprosima please test mac

@JesusPoderoso
Copy link
Contributor

Windows CI errors seems unrelated to the PR. Ready to merge

@JesusPoderoso JesusPoderoso 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 Jul 11, 2023
@JesusPoderoso
Copy link
Contributor

@Mergifyio backport 2.10.x 2.9.x

@mergify
Copy link
Contributor

mergify bot commented Jul 11, 2023 •
edited
Loading

backport 2.10.x 2.9.x

✅ Backports have been created

@MiguelCompany MiguelCompany merged commit e7720ca into master Jul 11, 2023
6 of 8 checks passed
@MiguelCompany MiguelCompany deleted the hotfix/best_effort_writer_udp_traffic branch July 11, 2023 05:38
mergify bot pushed a commit that referenced this pull request Jul 11, 2023
@juanlofer-eprosima @mergify
Fix StatelessWriter locators filtering (#3655)
e20cef0 
* Refs #18950: Introduce regression test

Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>

* Refs #18950: Make test build

Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>

* Finalize test

Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>

* Add fix

Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>

* Uncrustify

Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>

* Apply suggestions

Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>

* Separate parametrized tests

Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>

* Issue test compilation warning

Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>

* Make messages_sent an object attribute

Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>

* Get rid of messages_sent and use object's locator_filter

Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>

* Apply suggestion

Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>

---------

Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>
Co-authored-by: JesusPoderoso <jesuspoderoso@eprosima.com>
(cherry picked from commit e7720ca)
@mergify mergify bot mentioned this pull request Jul 11, 2023
Merged
mergify bot pushed a commit that referenced this pull request Jul 11, 2023
@juanlofer-eprosima @mergify
Fix StatelessWriter locators filtering (#3655)
ea604e2 
* Refs #18950: Introduce regression test

Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>

* Refs #18950: Make test build

Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>

* Finalize test

Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>

* Add fix

Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>

* Uncrustify

Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>

* Apply suggestions

Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>

* Separate parametrized tests

Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>

* Issue test compilation warning

Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>

* Make messages_sent an object attribute

Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>

* Get rid of messages_sent and use object's locator_filter

Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>

* Apply suggestion

Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>

---------

Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
Signed-off-by: Juan López Fernández <juanlopez@eprosima.com>
Co-authored-by: JesusPoderoso <jesuspoderoso@eprosima.com>
(cherry picked from commit e7720ca)
@mergify mergify bot mentioned this pull request Jul 11, 2023
Merged
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
ready-to-merge Ready to be merged. CI and changes have been reviewed and approved.
Projects
None yet
Milestone
v2.11.1
Development

Successfully merging this pull request may close these issues.
4 participants
@juanlofer-eprosima @MiguelCompany @JesusPoderoso @EduPonz