-
Notifications
You must be signed in to change notification settings - Fork 224
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
Added option and implementation for setting speedscaling factor via topic #883
base: main
Are you sure you want to change the base?
Conversation
formatting Allow changing topic name, made topic transient_local
@RobertWilbrandt might want to review this patch ? |
@fmauch and @RobertWilbrandt ? |
@fmauch and @RobertWilbrandt can you please take a look at the PR (and the other one related to it). I start running into merge conflicts now. I think the changes are not too large and are now tested on our system for roughly 2 months. |
I thought about how to react to this yesterday. As we are working on integrating this into the upstream JTC simultaneously and there we use a service to set the speed scaling I'm not that keen to implement another interface here that we will have to deprecate sooner or later. If you could rewrite this to be a service (and use the same parameters) as in the linked commit, I would not hesitate to merge this. |
Could you elaborate why you decided to use a service instead of a latched topic. I think a latched has various advantageous such as
Especially the first point is very important for our application where we have around 3-5 (scaled)JTCs running at the same time. If I were to you a service I would have to make sure to set the current scaling factor to each controller. From my point of view it would make more sense to suggest using a latched topic upstream.. |
Thank you for the elaboration, we will discuss this upstream. Unfortunately I don't have an open PR to discuss this atm, as I still want to bring it to a more shaped state, first. But I'll add it to my list :-) |
Same as #871 but based on main