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

[15168] Notify changes in bulk on RTPS readers #2842

Merged
merged 5 commits into from
Jul 26, 2022
Merged

[15168] Notify changes in bulk on RTPS readers #2842

merged 5 commits into from
Jul 26, 2022

Conversation

MiguelCompany
Copy link
Member

Description

This PR implements calls to the new on_data_available callback of rtps::ReaderListener

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.
  • N/A Any new/modified methods have been properly documented using Doxygen.
  • Fast DDS test suite has been run locally.
  • Changes are ABI compatible.
  • Changes are API compatible.
  • N/A Documentation builds and tests pass locally.
  • 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.

Reviewer Checklist

  • 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 MiguelCompany added the no-aarch Skip build & test for aarch64 label Jul 14, 2022
@MiguelCompany MiguelCompany force-pushed the feature/rtps-reader/bulk-notify branch 2 times, most recently from 93ba463 to 9e20873 Compare July 21, 2022 06:27
@MiguelCompany
Refs #14812. Fix order of cache changes.
db0a813 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany
Refs #15168. Changes on StatelessReader.
54df0d8 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany
Refs #15168. Changes on StatefulReader.
9d54343 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany MiguelCompany force-pushed the feature/rtps-reader/bulk-notify branch from 9e20873 to bc402d3 Compare July 21, 2022 07:08
@MiguelCompany MiguelCompany added this to the v2.7.1 milestone Jul 22, 2022
@MiguelCompany MiguelCompany marked this pull request as ready for review July 22, 2022 07:59
@MiguelCompany
Copy link
Member Author

@richiprosima Please test this

JLBuenoLopez
JLBuenoLopez previously approved these changes Jul 22, 2022
View reviewed changes
Copy link
Contributor

@JLBuenoLopez JLBuenoLopez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM with CI!

I assume it is not easy to prepare a deterministic test that ensures that some times a single message is notified and other times changes in a bulk are notified.

@MiguelCompany
Copy link
Member Author

@richiprosima Please test mac

@MiguelCompany MiguelCompany force-pushed the feature/rtps-reader/bulk-notify branch 3 times, most recently from 6370e13 to ccdc970 Compare July 22, 2022 17:21
@MiguelCompany
Copy link
Member Author

@richiprosima Please test mac

@MiguelCompany MiguelCompany mentioned this pull request Jul 22, 2022
Merged
13 tasks
@MiguelCompany
Copy link
Member Author

@richiprosima Please test windows and test linux also

@MiguelCompany
Refs #15168. Update state before notifying.
635cb83 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany MiguelCompany force-pushed the feature/rtps-reader/bulk-notify branch from ccdc970 to 635cb83 Compare July 23, 2022 09:06
@MiguelCompany
Copy link
Member Author

@richiprosima Please test this

@MiguelCompany
Copy link
Member Author

@richiware Would you mind reviewing this?

@MiguelCompany
Copy link
Member Author

@richiprosima Please test this

@MiguelCompany MiguelCompany force-pushed the feature/rtps-reader/bulk-notify branch 2 times, most recently from e3f24b2 to f7120fb Compare July 26, 2022 12:25
@MiguelCompany
Refs #15168. Added unit test.
dc35cef 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany MiguelCompany force-pushed the feature/rtps-reader/bulk-notify branch from f7120fb to dc35cef Compare July 26, 2022 12:29
@MiguelCompany
Copy link
Member Author

@richiprosima Please test this

@MiguelCompany
Copy link
Member Author

@richiprosima Please test windows

@MiguelCompany
Copy link
Member Author

@richiprosima Please test mac

@MiguelCompany MiguelCompany merged commit 2e45979 into master Jul 26, 2022
@MiguelCompany MiguelCompany deleted the feature/rtps-reader/bulk-notify branch July 26, 2022 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richiware richiware richiware approved these changes

@JLBuenoLopez JLBuenoLopez JLBuenoLopez left review comments

Assignees
No one assigned
Labels
no-aarch Skip build & test for aarch64
Projects
None yet
Milestone
v2.7.1
Development

Successfully merging this pull request may close these issues.
3 participants
@MiguelCompany @richiware @JLBuenoLopez