-
Notifications
You must be signed in to change notification settings - Fork 12
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
Call publishPendingTags on all blocks rather than this scheduler #428
base: main
Are you sure you want to change the base?
Call publishPendingTags on all blocks rather than this scheduler #428
Conversation
I see. This is not actually a |
I totally agree that this looks fishy, thanks for calling it out. 👍 I assume this was copied from a method inside of Block and then - when changing it to iterating over the blocks turned out to be non-trivial due to the needed APIs not exposed to the dynamically typed API - left this way and the required follow-up was missed. Should definitely have had a big I didn't fully follow it up now, but probably the way to go would be to only trigger the state change and implement publishing all pending tags in the Block's state handler and remove it from the scheduler. |
I believe it should be possible to |
I discussed with @drslebedev and we agreed that with the recent changes in tag handling and considering the fact that the line in it's current form is not doing anything since the scheduler does not have any ports, the correct solution is to remove the line alltogether. I'll update this PR to that effect. |
The call was calling the method on the scheduler itself instead of on the blocks it was iterating over. This was probably an incomplete refactoring in moving this functionality from the block to the Scheduler. Since then the tag handling was changed considerably, so that in the meantime this line can be safely removed. Thanks to @mattkretz for spotting this and starting the initial investigation. Signed-off-by: Alexander Krimm <[email protected]>
f08ae02
to
49a96f8
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 as discusses privately with @wirew0rm. PR can be merged.
The scheduler is derived from class SchedulerBase : public Block<Derived> {
friend class lifecycle::StateMachine<Derived>;
using JobLists = std::shared_ptr<std::vector<std::vector<BlockModel*>>>; |
The call was originally introduced with 49adc04.
I'm not sure that my change reflects the intention. If it isn't, this needs a comment IMHO.