Simplifying Scheduler
AbandonedPublic

Authored by wreissenberger on Mar 23 2019, 3:13 PM.

Details

Summary

Currently, the scheduler is a class with more than 7200 lines of code which makes it very tricky to maintain. This approach follows the idea to create a separate class ScheduleStrategy that holds the entire logic to select a certain schedule to be executed.

As a first step, the scoring calculation is extracted and shifted to ScheduleStrategy.

Test Plan

Well, this is the downside of this refactoring effort. Since the Scheduler is awfully complex and intertwined, it needs to be tested carefully.

At least from the intention, the execution of scheduler jobs should not be touched, only the correct calculations of scores. So please test all combinations that have effect to the score of a scheduler job.

Diff Detail

Repository
R321 KStars
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10014
Build 10032: arc lint + arc unit
wreissenberger created this revision.Mar 23 2019, 3:13 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptMar 23 2019, 3:13 PM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
wreissenberger requested review of this revision.Mar 23 2019, 3:13 PM

I like this approach. Not only the scheduler, but alignment module too has gotten out of control and is very complex now. But this has to be tested carefully as you suggested.

OK mostly, I'd rather have calculation pertaining to SchedulerJob relocated in SchedulerJob. "Strategy" would mean we have multiple ways of scheduling, which is not really the case today. We have "Restrictions" that schedules must comply with, and we have calculations leading to a score. I'm not a fan of that "score" value, but it pertains to the SchedulerJob, it should be the one owning the calculation. I think your previous work in moving stuff there was more interesting (but I can't find it anywhere...).
Let me add an additional comment later on, I'm a bit busy now.

I have done both. But relocating the functionality to SchedulerJob has the problem, that calculations for weather and darkness are not specific to a scheduler job. In fact, weather and darkness are properties of the entire schedule, not of the single scheduler job.
And yes, the idea points exactly in that direction that we may have several ones. In fact we have two: one where the sequence is set manually and one where the sequence is sorted by altitude. But both use the same scoring strategy. See ScheduleStrategy as base class.

I fully agree, but that means darkness and weather should go in ScheduleStrategy, and altitude score and moon separation score calculations should go to SchedulerJob.
Also, are you sure you need a separate class for now? Could you keep the code in Scheduler, but move it to a different source file?
That would prepare the codebase with zero risk of regression, and discuss what goes where. scheduler_strategy.cpp and scheduler_schedulerjob.cpp?

Alternately, we could say that if calculating a score, when all is needed from SchedulerJob is one characteristic, perhaps the code may take that characteristic instead of passing the full SchedulerJob.
That would reduce the inter-dependencies. In these conditions, we see that the altitude score calculation is not dependent on SchedulerJob, but on altitude and time calculation should be done at.

Right, that's what I first thought. But does it really make sense that a scheduler job rates its own score? Shouldn't be the rating of a job something that we would like to keep outside of a single job? That's why I came up with the idea to shift it to a separate class being associated with the Scheduler keeping the scoring logic.

And, btw. the risk of regression is similar. In fact, shifting to ScheduleStrategy was even simpler, since I could keep the method signatures. When shifting methods to SchedulerJob, the SchedulerJob as argument needs to be removed and the method calls adapted.

I agree with your points, but I still tend to think SchedulerJob should be able to produce a score by itself, answering "job, what's your score?".
SchedulerJob knows when it starts, knows the restrictions, and there's no added value in Scheduler for this purpose that I can see. Scheduler is just a tunnel providing global variables in most cases.
In the case of darkness, Scheduler needs the startup time of SchedulerJob. For dawn and dusk, when that will be fixed, Scheduler will still need the startup and completion time of SchedulerJob. Same again for Moon separation.
Now, I agree, altitude calculations need SchedulerJob times and non-Scheduler global variables, but also require a startup time that pertains to the previous job in the schedule. So, my opinion is obviously criticizable.
As a side note, scoring is only a can/cannot run flag in the end, and would better be completely removed at some point. That would spare us some hair.

Now, no problem, we can go for a utility section. But in that case, in my opinion there's no reason to add a new class. We can partition the code per responsibilities, then later on decide on what to cut off, where from and where to.
We could go for a "scheduler.cpp" with UI, "scheduler_strategy.cpp" with the job evaluation algorithm, "scheduler_frames.cpp" with the frame counting headache, "scheduler_restrictions.cpp" with the score/altitude/moon/darkness calculations, be it for the whole schedule or only jobs.

OK, @TallFurryMan , I give up :-) Here is the alternative with the score calculations shifted to SchedulerJob: D20068.

Oh no I'm sorry if I sounded too annoying to you, that wasn't my intent. Let me propose a preliminary differential too based on your work last year and with the same idea as this one. And then we both hit it until it has the shape of a sports car (well hopefully).

@TallFurryMan: Absolutely not, that's the nature of discussion. And I appreciate the ambition of making a sports car out of it :-)

wreissenberger abandoned this revision.May 2 2019, 6:51 PM

We follow another path with D20068 - closed.