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

[20401] Check History QoS inconsistencies (backport #4375) #4408

Merged
merged 6 commits into from
Apr 10, 2024
Merged

[20401] Check History QoS inconsistencies (backport #4375) #4408

merged 6 commits into from
Apr 10, 2024

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Feb 20, 2024

This is an automatic backport of pull request #4375 done by Mergify.
Cherry-pick of 68acb5a has failed:

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

You are currently cherry-picking commit 68acb5a6c.
  (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)

Changes to be committed:
	modified:   test/blackbox/common/DDSBlackboxTestsDataWriter.cpp
	modified:   test/unittest/dds/publisher/DataWriterTests.cpp
	modified:   test/unittest/dds/subscriber/DataReaderTests.cpp

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   src/cpp/fastdds/publisher/DataWriterImpl.cpp
	both modified:   src/cpp/fastdds/subscriber/DataReaderImpl.cpp
	both modified:   test/unittest/dds/profiles/test_xml_profiles.xml

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 commands and options

More conditions and actions can be found in the documentation.

You can also trigger Mergify actions by commenting on this pull request:

  • @Mergifyio refresh will re-evaluate the rules
  • @Mergifyio rebase will rebase this PR on its base branch
  • @Mergifyio update will merge the base branch into this PR
  • @Mergifyio backport <destination> will backport this PR on <destination> branch

Additionally, on Mergify dashboard you can:

  • look at your merge queues
  • generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com

@mergify mergify bot added the conflicts Backport PR wich git cherry pick failed label Feb 20, 2024
@mergify mergify bot mentioned this pull request Feb 20, 2024
Merged
12 tasks
@JesusPoderoso JesusPoderoso added this to the v2.6.8 milestone Feb 20, 2024
@JesusPoderoso JesusPoderoso added ci-pending PR which CI is running and removed conflicts Backport PR wich git cherry pick failed labels Feb 20, 2024
JesusPoderoso
JesusPoderoso previously approved these changes Feb 20, 2024
View reviewed changes
@Mario-DL Mario-DL mentioned this pull request Feb 21, 2024
Merged
7 tasks
JesusPoderoso
JesusPoderoso previously approved these changes Feb 22, 2024
View reviewed changes
@JesusPoderoso JesusPoderoso added skip-ci Automatically pass CI to-do and removed skip-ci Automatically pass CI labels Feb 22, 2024
@EduPonz EduPonz removed the to-do label Feb 24, 2024
EduPonz
EduPonz previously approved these changes Feb 24, 2024
View reviewed changes
@Mario-DL
Copy link
Member

@Mergifyio rebase 2.6.x

JesusPoderoso and others added 4 commits March 19, 2024 06:55
@JesusPoderoso @Mario-DL
Check History QoS inconsistencies (#4375)
822f237 
* Refs #20401: Add regression test

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

* Refs #20401: Check History QoS inconsistencies

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

* Refs #20401: Check also the history depth vs resource limits max_sample_per_instance bounds

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

* Refs #20401: Update HeartbeatWhileDestruction test

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

* Refs #20401: Update unit test DDS profiles XML tests profile

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

---------

Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
(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
@JesusPoderoso @Mario-DL
Refs #20401: Fix conflicts
1cb8cf4 
Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
@Mario-DL
Fix SecureHelloworldExample (#4416)
614ac65 
Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>
@MiguelCompany @EduPonz @Mario-DL
Just show warning when inconsistency between depth and max_samples_pe…
c29020b 
…r_instance (#4417)

* Refs #20503. Add regression test

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

* Refs #20503. Show warning when depth > max_samples_per_instance

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

* Refs #20503. Fix InvalidQos tests

Signed-off-by: EduPonz <eduardoponz@eprosima.com>

---------

Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Co-authored-by: EduPonz <eduardoponz@eprosima.com>
@mergify Mergify
Copy link
Contributor Author

mergify bot commented Mar 19, 2024

rebase 2.6.x

✅ Branch has been successfully rebased

@Mario-DL
Copy link
Member

@richiprosima please test this

@JesusPoderoso
Refs #20401: Fix warning
33103f1 
Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
Mario-DL
Mario-DL previously approved these changes Mar 20, 2024
View reviewed changes
Copy link
Member

@Mario-DL Mario-DL 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

@Mario-DL
Copy link
Member

@richiprosima please test this

@Mario-DL
Copy link
Member

Failed tests are unrelated to this PR and cis are green on Jenkins (except mac). Giving it ready to merge

@Mario-DL 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 Mar 21, 2024
@EduPonz
Copy link

EduPonz commented Mar 22, 2024

The following tests have been added in this PR and failed on Gthub macOS CI:

  • DataWriterTests.history_depth_max_samples_per_instance_warning
  • DataReaderTests.history_depth_max_samples_per_instance_warning

@EduPonz EduPonz added ci-pending PR which CI is running and removed ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. labels Mar 22, 2024
@Mario-DL
Copy link
Member

Mario-DL commented Mar 25, 2024 •
edited
Loading

Good catch ! Could you or @JesusPoderoso please take a look at this ?
(It is true that it did not fail in jenkins)

@Mario-DL
Copy link
Member

Mario-DL commented Apr 4, 2024

Friendly ping
CC: @JesusPoderoso @EduPonz

@JesusPoderoso
Refs #20401: Fix segfault in Mac tests
15ed027 
Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com>
@JesusPoderoso
Copy link
Contributor

@richiprosima please test this

@JesusPoderoso
Copy link
Contributor

Changes introduced in 15ed027 fixed locally.

@Mario-DL
Copy link
Member

Mario-DL commented Apr 5, 2024 •
edited
Loading

It is weird, the behavior of delete_contained_entities() should be the same as manually removing the entities. The segfault in delete_contained_entities() may have to do with destructing the listener while being executed. But it is true that changing it in that way fixes the test.
Will reorder jenkins windows ci since it took 6 hours and it may not be significative.

@Mario-DL
Copy link
Member

Mario-DL commented Apr 5, 2024

@richiprosima please test windows test mac

@Mario-DL
Copy link
Member

Mario-DL commented Apr 8, 2024

@richiprosima please test mac

1 similar comment
@Mario-DL
Copy link
Member

Mario-DL commented Apr 8, 2024

@richiprosima please test mac

@JesusPoderoso
Copy link
Contributor

It is weird, the behavior of delete_contained_entities() should be the same as manually removing the entities. The segfault in delete_contained_entities() may have to do with destructing the listener while being executed.

Agree on that, it should work anyways...

@Mario-DL
Copy link
Member

Mario-DL commented Apr 9, 2024

@richiprosima please test mac

1 similar comment
@Mario-DL
Copy link
Member

@richiprosima please test mac

@Mario-DL
Copy link
Member

Failed github tests are unrelated. This time we are not waiting for jenkins ci since it is not stable right now and it cannot block an entire week. Ready to merge

@Mario-DL 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 Apr 10, 2024
@EduPonz EduPonz merged commit cca4e18 into 2.6.x Apr 10, 2024
12 of 16 checks passed
@EduPonz EduPonz deleted the mergify/bp/2.6.x/pr-4375 branch April 10, 2024 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JesusPoderoso JesusPoderoso JesusPoderoso left review comments

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

@EduPonz EduPonz EduPonz left review comments

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.6.8
Development

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