-
Notifications
You must be signed in to change notification settings - Fork 787
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
[20815] Only apply content filter to ALIVE changes (backport #4876) #4902
Conversation
Cherry-pick of 9a64956 has failed:
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 |
* Refs #20815: Add regression test Signed-off-by: eduponz <[email protected]> * Refs #20815: Only apply filter to ALIVE changes Signed-off-by: eduponz <[email protected]> * Refs #20815: Rename change_is_relevant_for_filter argument Signed-off-by: eduponz <[email protected]> * Refs #20815: Refactor test Signed-off-by: eduponz <[email protected]> * Refs #20815: Cast loop index to uint16_t for assigning it to the key field Signed-off-by: eduponz <[email protected]> * Refs #20815: Refactor test so PubSubWriter can be used directly Signed-off-by: eduponz <[email protected]> * Refs #20815: Fix memory leak Signed-off-by: eduponz <[email protected]> * Refs #20815: Apply Mario's suggestions Signed-off-by: eduponz <[email protected]> * Refs #20815: Add type traits to PubSubWriterReader and PubSubParticipant Signed-off-by: eduponz <[email protected]> * Refs #20815: Default move ctor and assignment in DynamicLoanableSequence Signed-off-by: eduponz <[email protected]> * Refs #20815: Add doxygen in DynamicLoanableSequence Signed-off-by: eduponz <[email protected]> * Refs #20815: Delete copy semantic from DynamicLoanableSequence Signed-off-by: eduponz <[email protected]> * Refs #20815: Use alias for TypeTraits::DataListType in PubSub* classes Signed-off-by: eduponz <[email protected]> --------- Signed-off-by: eduponz <[email protected]> (cherry picked from commit 9a64956) Signed-off-by: eduponz <[email protected]>
45929f5
to
675590d
Compare
…d functions Signed-off-by: eduponz <[email protected]>
675590d
to
a9f60cd
Compare
There was a problem hiding this 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
@richiprosima please test this |
Signed-off-by: eduponz <[email protected]>
a9f60cd
to
0258c6c
Compare
@richiprosima please test linux |
Description
Backport of #4835 to 2.x
This PR fixes a bug that caused the content filter to also be applied to
unregister
anddisposed
samples. Since in those messages the only fields populated (if any) are the ones annotated with@key
, theunregister
anddispose
samples did not pass the filter (in general) and thus were being discarded. This caused several issues:unregister
ordispose
followed by awrite
were triggeringsample_lost
events, as the received sequence numbers were not consecutive (because of the filtering out of theunregister
/dispose
).This PR fixes these issues by only querying for sample relevance when the
CacheChange
kind isALIVE
.@Mergifyio backport 2.13.x 2.10.x 2.6.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
This is an automatic backport of pull request #4876 done by [Mergify](https://mergify.com).