-
Notifications
You must be signed in to change notification settings - Fork 783
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 #4835
Conversation
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.
This PR brings an interesting bugfix
. Leaving a couple of suggestions
51eac25
to
96841b0
Compare
Signed-off-by: eduponz <eduardoponz@eprosima.com>
Signed-off-by: eduponz <eduardoponz@eprosima.com>
Signed-off-by: eduponz <eduardoponz@eprosima.com>
Signed-off-by: eduponz <eduardoponz@eprosima.com>
…field Signed-off-by: eduponz <eduardoponz@eprosima.com>
96841b0
to
d01e6ac
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
Backport to 2.14.x: The rest of the backport will be ordered from there. |
Description
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
.@EduPonz backport 2.14.x 2.13.x 2.10.x 2.6.x
Contributor Checklist
versions.md
file (if applicable).Reviewer Checklist