[20504] Just show warning when inconsistency between depth and max_samples_per_instance #4417

MiguelCompany · 2024-02-21T16:22:09Z

Description

This addresses a regression in ROS 2 (ros2/ros2#1527) after #4375 was merged.

Whenever a DataWriter is created taking the default DataWriterQos, and changing the history depth to a value greater than 400, the creation of the DataWriter fails. Same happens when creating a DataReader.

This PR partially reverts #4375, so the inconsistency is shown as a warnings instead of making the entity creation fail.

@Mergifyio backport 2.10.x

*For the other branches, we'll include this as part of the corresponding backport of #4375)

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: [20503] Clarify history depth vs max_samples_per_instance inconsistency Fast-DDS-docs#679
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.
Check CI results: changes do not issue any warning.
Check CI results: failing tests are unrelated with the changes.

MiguelCompany · 2024-02-21T16:27:50Z

@clalancette This should fix the regression mentioned in ros2/ros2#1527

Could you please check?

EduPonz · 2024-02-22T10:25:21Z

@richiprosima please test this

JesusPoderoso

LGTM with green CI

EduPonz · 2024-02-22T12:01:53Z

@Mergifyio backport 2.10.x

mergify · 2024-02-22T12:01:56Z

backport 2.10.x

✅ Backports have been created

#4440 [20504] Just show warning when inconsistency between depth and max_samples_per_instance (backport #4417) has been created for branch 2.10.x but encountered conflicts

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

JesusPoderoso · 2024-02-22T13:57:50Z

@richiprosima please test this

EduPonz · 2024-02-22T17:42:24Z

@richiprosima please test windows

clalancette · 2024-02-22T19:39:25Z

Here is ROS 2 CI on this change:

Linux
Linux-aarch64
Windows
Windows Debug
RHEL
Clang

…r_instance (#4417) * Refs #20503. Add regression test Signed-off-by: EduPonz <[email protected]> * Refs #20503. Show warning when depth > max_samples_per_instance Signed-off-by: EduPonz <[email protected]> * Refs #20503. Fix InvalidQos tests Signed-off-by: EduPonz <[email protected]> --------- Signed-off-by: EduPonz <[email protected]> Co-authored-by: EduPonz <[email protected]> (cherry picked from commit 12201f4)

…r_instance (#4417) (#4440) * Refs #20503. Add regression test Signed-off-by: EduPonz <[email protected]> * Refs #20503. Show warning when depth > max_samples_per_instance Signed-off-by: EduPonz <[email protected]> * Refs #20503. Fix InvalidQos tests Signed-off-by: EduPonz <[email protected]> --------- Signed-off-by: EduPonz <[email protected]> Co-authored-by: EduPonz <[email protected]> (cherry picked from commit 12201f4) Co-authored-by: Miguel Company <[email protected]>

…r_instance (#4417) * Refs #20503. Add regression test Signed-off-by: EduPonz <[email protected]> * Refs #20503. Show warning when depth > max_samples_per_instance Signed-off-by: EduPonz <[email protected]> * Refs #20503. Fix InvalidQos tests Signed-off-by: EduPonz <[email protected]> --------- Signed-off-by: EduPonz <[email protected]> Co-authored-by: EduPonz <[email protected]>

* Check History QoS inconsistencies (#4375) * Refs #20401: Add regression test Signed-off-by: JesusPoderoso <[email protected]> * Refs #20401: Check History QoS inconsistencies Signed-off-by: JesusPoderoso <[email protected]> * Refs #20401: Check also the history depth vs resource limits max_sample_per_instance bounds Signed-off-by: JesusPoderoso <[email protected]> * Refs #20401: Update HeartbeatWhileDestruction test Signed-off-by: JesusPoderoso <[email protected]> * Refs #20401: Update unit test DDS profiles XML tests profile Signed-off-by: JesusPoderoso <[email protected]> --------- Signed-off-by: JesusPoderoso <[email protected]> (cherry picked from commit 68acb5a) * Fix SecureHelloworldExample (#4416) Signed-off-by: Mario Dominguez <[email protected]> * Just show warning when inconsistency between depth and max_samples_per_instance (#4417) * Refs #20503. Add regression test Signed-off-by: EduPonz <[email protected]> * Refs #20503. Show warning when depth > max_samples_per_instance Signed-off-by: EduPonz <[email protected]> * Refs #20503. Fix InvalidQos tests Signed-off-by: EduPonz <[email protected]> --------- Signed-off-by: EduPonz <[email protected]> Co-authored-by: EduPonz <[email protected]> --------- Signed-off-by: Mario Dominguez <[email protected]> Signed-off-by: EduPonz <[email protected]> Co-authored-by: Jesús Poderoso <[email protected]> Co-authored-by: Mario Domínguez López <[email protected]> Co-authored-by: Miguel Company <[email protected]> Co-authored-by: EduPonz <[email protected]>

…r_instance (#4417) * Refs #20503. Add regression test Signed-off-by: EduPonz <[email protected]> * Refs #20503. Show warning when depth > max_samples_per_instance Signed-off-by: EduPonz <[email protected]> * Refs #20503. Fix InvalidQos tests Signed-off-by: EduPonz <[email protected]> --------- Signed-off-by: EduPonz <[email protected]> Co-authored-by: EduPonz <[email protected]>

* Check History QoS inconsistencies (#4375) * Refs #20401: Add regression test Signed-off-by: JesusPoderoso <[email protected]> * Refs #20401: Check History QoS inconsistencies Signed-off-by: JesusPoderoso <[email protected]> * Refs #20401: Check also the history depth vs resource limits max_sample_per_instance bounds Signed-off-by: JesusPoderoso <[email protected]> * Refs #20401: Update HeartbeatWhileDestruction test Signed-off-by: JesusPoderoso <[email protected]> * Refs #20401: Update unit test DDS profiles XML tests profile Signed-off-by: JesusPoderoso <[email protected]> --------- Signed-off-by: JesusPoderoso <[email protected]> (cherry picked from commit 68acb5a) # Conflicts: # src/cpp/fastdds/publisher/DataWriterImpl.cpp # src/cpp/fastdds/subscriber/DataReaderImpl.cpp # test/unittest/dds/profiles/test_xml_profiles.xml * Refs #20401: Fix conflicts Signed-off-by: JesusPoderoso <[email protected]> * Fix SecureHelloworldExample (#4416) Signed-off-by: Mario Dominguez <[email protected]> * Just show warning when inconsistency between depth and max_samples_per_instance (#4417) * Refs #20503. Add regression test Signed-off-by: EduPonz <[email protected]> * Refs #20503. Show warning when depth > max_samples_per_instance Signed-off-by: EduPonz <[email protected]> * Refs #20503. Fix InvalidQos tests Signed-off-by: EduPonz <[email protected]> --------- Signed-off-by: EduPonz <[email protected]> Co-authored-by: EduPonz <[email protected]> * Refs #20401: Fix warning Signed-off-by: JesusPoderoso <[email protected]> * Refs #20401: Fix segfault in Mac tests Signed-off-by: JesusPoderoso <[email protected]> --------- Signed-off-by: JesusPoderoso <[email protected]> Signed-off-by: Mario Dominguez <[email protected]> Signed-off-by: EduPonz <[email protected]> Co-authored-by: Jesús Poderoso <[email protected]> Co-authored-by: JesusPoderoso <[email protected]> Co-authored-by: Mario Domínguez López <[email protected]> Co-authored-by: Miguel Company <[email protected]> Co-authored-by: EduPonz <[email protected]>

MiguelCompany added this to the v2.13.3 milestone Feb 21, 2024

MiguelCompany changed the title ~~Change default resource limits to infinite~~ [20504] Change default resource limits to infinite Feb 21, 2024

EduPonz force-pushed the hotfix/default-unlimited-resources branch from 19cad85 to 372434d Compare February 21, 2024 19:27

EduPonz mentioned this pull request Feb 21, 2024

[20503] Clarify history depth vs max_samples_per_instance inconsistency eProsima/Fast-DDS-docs#679

Merged

EduPonz previously approved these changes Feb 21, 2024

View reviewed changes

EduPonz dismissed their stale review via 0dbcfe5 February 22, 2024 10:21

EduPonz force-pushed the hotfix/default-unlimited-resources branch from 372434d to 0dbcfe5 Compare February 22, 2024 10:21

EduPonz changed the title ~~[20504] Change default resource limits to infinite~~ [20504] Just show warning when inconsistency between depth and max_samples_per_instance Feb 22, 2024

JesusPoderoso previously approved these changes Feb 22, 2024

View reviewed changes

EduPonz added the ci-pending PR which CI is running label Feb 22, 2024

EduPonz added 3 commits February 22, 2024 14:48

Refs #20503. Add regression test

a25eb42

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

Refs #20503. Show warning when depth > max_samples_per_instance

681b7c0

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

Refs #20503. Fix InvalidQos tests

dc8d336

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

EduPonz dismissed JesusPoderoso’s stale review via dc8d336 February 22, 2024 13:49

EduPonz force-pushed the hotfix/default-unlimited-resources branch from 0dbcfe5 to dc8d336 Compare February 22, 2024 13:49

JesusPoderoso approved these changes Feb 22, 2024

View reviewed changes

This was referenced Feb 22, 2024

[20414] Bump version to 2.14.0 #4439

Merged

[20401] Check History QoS inconsistencies #4375

Merged

EduPonz merged commit 12201f4 into master Feb 23, 2024

EduPonz deleted the hotfix/default-unlimited-resources branch February 23, 2024 06:04

mergify bot mentioned this pull request Feb 23, 2024

[20504] Just show warning when inconsistency between depth and max_samples_per_instance (backport #4417) #4440

Merged

MiguelCompany mentioned this pull request Feb 27, 2024

Revert "Pin Fast-DDS to the last known working version." ros2/ros2#1529

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[20504] Just show warning when inconsistency between depth and max_samples_per_instance #4417

[20504] Just show warning when inconsistency between depth and max_samples_per_instance #4417

MiguelCompany commented Feb 21, 2024 •

edited by EduPonz

Loading

MiguelCompany commented Feb 21, 2024

EduPonz commented Feb 22, 2024

JesusPoderoso left a comment

EduPonz commented Feb 22, 2024

mergify bot commented Feb 22, 2024 •

edited

Loading

JesusPoderoso commented Feb 22, 2024

EduPonz commented Feb 22, 2024

clalancette commented Feb 22, 2024

[20504] Just show warning when inconsistency between depth and max_samples_per_instance #4417

[20504] Just show warning when inconsistency between depth and max_samples_per_instance #4417

Conversation

MiguelCompany commented Feb 21, 2024 • edited by EduPonz Loading

Description

Contributor Checklist

Reviewer Checklist

MiguelCompany commented Feb 21, 2024

EduPonz commented Feb 22, 2024

JesusPoderoso left a comment

Choose a reason for hiding this comment

EduPonz commented Feb 22, 2024

mergify bot commented Feb 22, 2024 • edited Loading

✅ Backports have been created

JesusPoderoso commented Feb 22, 2024

EduPonz commented Feb 22, 2024

clalancette commented Feb 22, 2024

MiguelCompany commented Feb 21, 2024 •

edited by EduPonz

Loading

mergify bot commented Feb 22, 2024 •

edited

Loading