-
Notifications
You must be signed in to change notification settings - Fork 193
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
Allow to reconfigure durability for /tf topic broadcaster/listener #383
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.
LGTM, but I'd like a second opinion.
@clalancette could you take a look to this? do you think splitting the old |
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 idea here seems fine to me; we split the /tf
and /tf_static
, so having them have different options and overrides make sense.
I can't say I love the implementation. In particular, void init()
is a private method, so there is no real reason for it to have overridable default arguments. I'd just move the code that creates the options
and static_options
into the body of init()
, and simplify the signature.
Good point! I didn't notice that it was a private method |
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
9970b9e
to
0ec7e9e
Compare
Actually, I need the defaults in the constructor and not only in |
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
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.
Looks good to me with green CI! Thanks for iterating.
Follow up of #381.
Allows reconfiguring the durability qos setting for /tf broadcaster/listener.
This wasn't applied to /tf_static, as in that case durability must always be transient local.