Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mattkretz
Copy link
Collaborator

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.

@mattkretz mattkretz added the bug Something isn't working label Sep 30, 2024
@mattkretz
Copy link
Collaborator Author

I see. This is not actually a Block<T>::derived_t type but a BlockModel. Then, what's the comment I could add to explain why I'm reading this all wrong? 😉

@wirew0rm
Copy link
Member

wirew0rm commented Sep 30, 2024

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 // DO NOT MERGE YET comment.

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.
On the other hand as it is, the code has been doing basically nothing all the time since it was introduced, so it might be also possible to drop it. I'll have a look.

@mattkretz
Copy link
Collaborator Author

I believe it should be possible to publishPendingTags on the ports of the block behind the BlockModel by introducing a new virtual function to BlockModel (implemented by BlockWrapper). Whether that's necessary 🤷

@wirew0rm
Copy link
Member

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]>
@wirew0rm wirew0rm force-pushed the publishPendingTags_on_blocks_rather_than_schedulers branch from f08ae02 to 49a96f8 Compare September 30, 2024 15:22
@wirew0rm wirew0rm requested review from drslebedev and removed request for wirew0rm September 30, 2024 15:22
Copy link
Contributor

@drslebedev drslebedev left a 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.

@RalphSteinhagen
Copy link
Member

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 scheduler is derived from Block<T> and inherits its message ports. The UI would trigger/accept messages from that port:

class SchedulerBase : public Block<Derived> {
    friend class lifecycle::StateMachine<Derived>;
    using JobLists = std::shared_ptr<std::vector<std::vector<BlockModel*>>>;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants