-
Notifications
You must be signed in to change notification settings - Fork 791
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
Fix partition copy in QoS [11451] #1947
Conversation
94ced56
to
fd802f3
Compare
Hi Miguel, I'm pretty new to this project, so please forgive the newbie questions. Do I need to assign reviewers to this somehow, or does that happen automatically? And should I worry about the CI failures? Seems like quite a few other PRs have failed CI as well. Thanks for the great project! |
@jay-sridharan First of all, thank you for your contribution. The process is that someone at eProsima reviews PRs, we will do it with yours soon. The CI failures seem unrelated to your changes, but checking them is also part of the review process. |
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.
The changes look good to me, but it would be great to have a regression test in place.
Would you mind adding one?
Also, while trying to find the root cause of the issue I've also found that these lines need a pit.second->clear()
before the corresponding push_back
Thanks for the review! I'll look into adding a regression test. Any thoughts off the top of your head as to the best way to do that? Probably set the resource limit to a single reader instance to limit the size of the pool, and then re-construct one and make sure the QoS was copied correctly? Perhaps there is an easier way |
Signed-off-by: Jay Sridharan <jsridharan@relativityspace.com>
fd802f3
to
b9a08f5
Compare
I have rebased and apply the review suggestion. Part of the changes suggested in this PR were already introduced in #2108. I have followed the same approach in the publisher's side. |
@richiprosima please test this |
@Mergifyio backport 2.10.x 2.9.x 2.6.x |
✅ Backports have been created
|
Signed-off-by: Jay Sridharan <jsridharan@relativityspace.com> Co-authored-by: Jay Sridharan <jsridharan@relativityspace.com> (cherry picked from commit 7bcbfa3)
Signed-off-by: Jay Sridharan <jsridharan@relativityspace.com> Co-authored-by: Jay Sridharan <jsridharan@relativityspace.com> (cherry picked from commit 7bcbfa3)
Signed-off-by: Jay Sridharan <jsridharan@relativityspace.com> Co-authored-by: Jay Sridharan <jsridharan@relativityspace.com> (cherry picked from commit 7bcbfa3)
This PR fixes a bug where a QoS is copied from one ReaderProxyData to another but partition names are not copied if the list is empty.
Specifically, this code path: