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.

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

[21519] Fix issue with exclusive ownership and unordered samples #5182

Merged
merged 5 commits into from
Sep 10, 2024

Conversation

MiguelCompany
Copy link
Member

@MiguelCompany MiguelCompany commented Aug 30, 2024

Description

During interoperability testing, some flakiness on Test_Ownership_3 unveiled that a reader with exclusive ownership and a KEEP_ALL history may return a mix of samples from writers with different strengths.

This PR adds a regression test and fixes the issue by removing inappropriate samples during the take operation.

@Mergifyio backport 2.14.x 2.10.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.
  • 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.

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.

Just a NIT.
Alternatively, we may have chosen to do the check when adding the change to the instance instead of "waiting" for the user to perform a read/take to do the cleanup, but it should be equivalent.

test/blackbox/common/DDSBlackboxTestsOwnershipQos.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot added the ci-pending PR which CI is running label Aug 30, 2024
Mario-DL
Mario-DL previously approved these changes Aug 30, 2024
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

@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 Sep 2, 2024
@MiguelCompany MiguelCompany added in progress Issue or PR which is being reviewed and removed ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. labels Sep 2, 2024
@MiguelCompany MiguelCompany force-pushed the bugfix/20866 branch 2 times, most recently from b13d1de to ac54175 Compare September 3, 2024 07:11
@MiguelCompany
Copy link
Member Author

@Mario-DL I have rebased this one, and also refactored both the test (I added a second one) and the fix (remove when adding).

@MiguelCompany MiguelCompany added needs-review PR that is ready to be reviewed and removed in progress Issue or PR which is being reviewed labels Sep 3, 2024
@github-actions github-actions bot added the ci-pending PR which CI is running label Sep 3, 2024
@MiguelCompany MiguelCompany requested review from Mario-DL and removed request for Mario-DL September 3, 2024 13:43
@JesusPoderoso JesusPoderoso requested review from Mario-DL and removed request for Mario-DL September 4, 2024 14:03
@MiguelCompany MiguelCompany removed the request for review from Mario-DL September 4, 2024 14:03
@MiguelCompany
Copy link
Member Author

Note: The failure in MacOS also failed in PR 5181, so it seems unrelated.

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.

Fine rework overall ! Leaving a question in the code and other one that comes to me.
If I understood correctly, the changes may affect also to a reader with a sufficiently big keep_last history (e.g 20 or 40).
If that's the case, would it be useful to include an analogous test with that history kind just to check ?

@Mario-DL
Copy link
Member

Mario-DL commented Sep 6, 2024

NIT
We may remove KEEP_ALL from the PR's title

@MiguelCompany MiguelCompany requested review from Mario-DL and removed request for Mario-DL September 6, 2024 10:50
@MiguelCompany MiguelCompany changed the title [21519] Fix issue with exclusive ownership and KEEP_ALL [21519] Fix issue with exclusive ownership and unordered samples Sep 6, 2024
@MiguelCompany
Copy link
Member Author

@Mario-DL I rebased this one and refactored the tests in commit b886d86

I also changed the title.

@JesusPoderoso JesusPoderoso removed needs-review PR that is ready to be reviewed ci-pending PR which CI is running labels Sep 10, 2024
Copy link
Contributor

@JesusPoderoso JesusPoderoso left a comment

Choose a reason for hiding this comment

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

LGTM

@JesusPoderoso JesusPoderoso added the ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. label Sep 10, 2024
@MiguelCompany MiguelCompany merged commit b1a7fe2 into master Sep 10, 2024
17 checks passed
@MiguelCompany MiguelCompany deleted the bugfix/20866 branch September 10, 2024 09:22
@MiguelCompany
Copy link
Member Author

@Mergifyio backport 2.14.x 2.10.x

Copy link
Contributor

mergify bot commented Sep 10, 2024

backport 2.14.x 2.10.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Sep 10, 2024
* Refs #20866. Regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Additional regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix issue.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix unit tests.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Refactor test to run several cases.

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>
(cherry picked from commit b1a7fe2)

# Conflicts:
#	test/unittest/dds/subscriber/DataReaderHistoryTests.cpp
mergify bot pushed a commit that referenced this pull request Sep 10, 2024
* Refs #20866. Regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Additional regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix issue.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix unit tests.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Refactor test to run several cases.

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>
(cherry picked from commit b1a7fe2)

# Conflicts:
#	test/unittest/dds/subscriber/DataReaderHistoryTests.cpp
JesusPoderoso pushed a commit that referenced this pull request Sep 10, 2024
* Refs #20866. Regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Additional regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix issue.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix unit tests.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Refactor test to run several cases.

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>
(cherry picked from commit b1a7fe2)
Signed-off-by: JesusPoderoso <[email protected]>

# Conflicts:
#	test/unittest/dds/subscriber/DataReaderHistoryTests.cpp
JesusPoderoso pushed a commit that referenced this pull request Sep 10, 2024
* Refs #20866. Regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Additional regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix issue.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix unit tests.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Refactor test to run several cases.

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>
(cherry picked from commit b1a7fe2)
Signed-off-by: JesusPoderoso <[email protected]>

# Conflicts:
#	test/unittest/dds/subscriber/DataReaderHistoryTests.cpp
JesusPoderoso pushed a commit that referenced this pull request Sep 10, 2024
* Refs #20866. Regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Additional regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix issue.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix unit tests.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Refactor test to run several cases.

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>
(cherry picked from commit b1a7fe2)
Signed-off-by: JesusPoderoso <[email protected]>

# Conflicts:
#	test/unittest/dds/subscriber/DataReaderHistoryTests.cpp
JesusPoderoso pushed a commit that referenced this pull request Sep 10, 2024
* Refs #20866. Regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Additional regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix issue.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix unit tests.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Refactor test to run several cases.

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>
(cherry picked from commit b1a7fe2)
Signed-off-by: JesusPoderoso <[email protected]>

# Conflicts:
#	test/unittest/dds/subscriber/DataReaderHistoryTests.cpp
JesusPoderoso pushed a commit that referenced this pull request Sep 10, 2024
* Refs #20866. Regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Additional regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix issue.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix unit tests.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Refactor test to run several cases.

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>
(cherry picked from commit b1a7fe2)
Signed-off-by: JesusPoderoso <[email protected]>

# Conflicts:
#	test/unittest/dds/subscriber/DataReaderHistoryTests.cpp
JesusPoderoso pushed a commit that referenced this pull request Sep 10, 2024
* Refs #20866. Regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Additional regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix issue.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix unit tests.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Refactor test to run several cases.

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>
(cherry picked from commit b1a7fe2)
Signed-off-by: JesusPoderoso <[email protected]>

# Conflicts:
#	test/unittest/dds/subscriber/DataReaderHistoryTests.cpp
MiguelCompany added a commit that referenced this pull request Sep 11, 2024
* Refs #20866. Regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Additional regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix issue.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix unit tests.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Refactor test to run several cases.

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>
(cherry picked from commit b1a7fe2)
Signed-off-by: JesusPoderoso <[email protected]>

# Conflicts:
#	test/unittest/dds/subscriber/DataReaderHistoryTests.cpp

Co-authored-by: Miguel Company <[email protected]>
MiguelCompany added a commit that referenced this pull request Sep 11, 2024
* Refs #20866. Regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Additional regression test.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix issue.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Fix unit tests.

Signed-off-by: Miguel Company <[email protected]>

* Refs #20866. Refactor test to run several cases.

Signed-off-by: Miguel Company <[email protected]>

---------

Signed-off-by: Miguel Company <[email protected]>
(cherry picked from commit b1a7fe2)
Signed-off-by: JesusPoderoso <[email protected]>

# Conflicts:
#	test/unittest/dds/subscriber/DataReaderHistoryTests.cpp

Co-authored-by: Miguel Company <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Ready to be merged. CI and changes have been reviewed and approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants