D22446: Adding error handling strategy control to Scheduler

[Eric Dejouhanet]

TallFurryMan requested changes to this revision.
TallFurryMan added a comment.
This revision now requires changes to proceed.


  Please see my comments. I'm on with the idea :)
  Have you seen my post on the forum about the future of scheduler? We need to talk about this at some point.

INLINE COMMENTS

> capture.cpp:625
> +    // module keeps the control.
> +    if (activeJob != nullptr || m_State == CAPTURE_SUSPENDED)
> +        emit newStatus(targetState);

OK, but why did you move the test?

> scheduler.cpp:1431
>      /* FIXME: jobs in state JOB_ERROR should not be in the list, reorder states */
>      QList<SchedulerJob *> sortedJobs = jobs;
>  

Here I tend to disagree. We have JOB_INVALID, JOB_ERROR and JOB_ABORTED.
JOB_INVALID indicates a configuration error, and we cannot proceed with the job at all.
JOB_ERROR indicates a fatal error during processing, and we cannot proceed with the job anymore.
JOB_ABORTED indicates a transitory error during processing, and we should retry at some point.
Based on these definitions, we should not re-evaluate JOB_ERROR.
Now, I know that the enums are not properly ordered, but to me JOB_ERROR jobs should still be removed.

> scheduler.cpp:1537
>  
> +    /* This predicate matches jobs neither being evaluated nor aborted nor in error state */
> +    auto neither_evaluated_nor_aborted_nor_error = [](SchedulerJob const * const job)

OK for this block, interesting feature indeed.

> scheduler.cpp:1579
> +
> +        if (errorHandlingRestartAfterAllButton->isChecked())
> +        {

I'm not sure about this block, it really gets in the way of the regular method of scheduling.
It will conflict with the completion time of the aborted jobs.
It will run a parallel conflict with whatever restriction makes the jobs aborted at some point.
It will conflict with the preemptive shutdown option (unless you forbid a larger delay?).
Eventually, forbid a delay larger than the lead time option?

> scheduler.cpp:1602
>      if (Options::sortSchedulerJobs())
>      {
>          using namespace std::placeholders;

OK, only if scheduling algorithm takes the presence of JOB_ABORTED into account.

> scheduler.cpp:2061
>      }
>  
>      /* Apply sorting to queue table, and mark it for saving if it changes */

OK, only if scheduling algorithm takes the presence of JOB_ABORTED into account.

> scheduler.cpp:3165
>                                 currentJob->getCompletionTime().toString(currentJob->getDateTimeDisplayFormat())));
> -            currentJob->setState(SchedulerJob::JOB_ABORTED);
> +            currentJob->setState(SchedulerJob::JOB_COMPLETE);
>              stopCurrentJobAction();

This is a behavior change, but well, makes sense now.
Note that currently, the completion date embeds both date and time.
The completion date should be optional, so that multi-day schedules could be more easily programmed (I have a need for this).
The interface is painful when rescheduling START_AT and COMPLETE_AT jobs, but not moving jobs should improve that.

> scheduler.cpp:3191
>  
> -                currentJob->setState(SchedulerJob::JOB_ABORTED);
> +                currentJob->setState(SchedulerJob::JOB_COMPLETE);
>                  stopCurrentJobAction();

I disagree: this job must be rescheduled to next observation time, thus JOB_ABORTED.

> scheduler.cpp:3217
>  
> -                currentJob->setState(SchedulerJob::JOB_ABORTED);
> +                currentJob->setState(SchedulerJob::JOB_COMPLETE);
>                  stopCurrentJobAction();

I disagree: this job must be rescheduled to next observation time (albeit quite a far one!), thus JOB_ABORTED.

> scheduler.cpp:3236
>                                preDawnDateTime.toString(), Options::preDawnTime(), currentJob->getName()));
> -            currentJob->setState(SchedulerJob::JOB_ABORTED);
> +            currentJob->setState(SchedulerJob::JOB_COMPLETE);
>              stopCurrentJobAction();

I disagree: this job must be rescheduled to the next observation time, thus JOB_ABORTED.

> scheduler.cpp:3892
>  
> +    // We expect all data read from the XML to be in the C locale - QLocale::c()
> +    QLocale cLocale = QLocale::c();

OK! I thought this was already in.

> scheduler.cpp:3910
>                  }
> +                else if (!strcmp(tag, "ErrorHandlingStrategy"))
> +                {

OK.

> scheduler.cpp:3912
> +                {
> +                    setErrorHandlingStrategy(static_cast<ErrorHandlingStrategy>(cLocale.toInt(findXMLAttValu(ep, "value"))));
> +

Enum-to-int conversions are dangerous, we should review this method at some point...

> scheduler.cpp:3922
> +                    {
> +                        errorHandlingRescheduleErrorsCB->setChecked(!strcmp(findXMLAttValu(subEP, "enabled"), "true"));
> +                    }

The default value "true" is a bit lost in the code here, can we do better?
Line 3942 for instance, startup procedure block, has the boolean flag present or not in the "StartupProcedure" element.
The default is clear at lines 3929. Wouldn't that be better for maintenance?

> scheduler.cpp:4459
>  
> -    if (currentJob->getState() == SchedulerJob::JOB_ERROR)
> +    if (currentJob->getState() == SchedulerJob::JOB_ERROR || currentJob->getState() == SchedulerJob::JOB_ABORTED)
>      {

As mentioned earlier, JOB_ERROR jobs should be left cancelled per definition.

> scheduler.cpp:4481
>  
> -        appendLogText(i18n("Job '%1' is aborted.", currentJob->getName()));
> +            appendLogText(i18n("Waiting %1 seconds to restart job '%2'.", errorHandlingDelaySB->value(), currentJob->getName()));
>  

Same issue as described earlier.

> scheduler.cpp:4491
>  
> -        setCurrentJob(nullptr);
> +        // otherwise start re-evaluation
>          schedulerTimer.start();

Comment on the reason of the setCurrentJob move?

> scheduler.cpp:5949
>  
> +Scheduler::ErrorHandlingStrategy Scheduler::getErrorHandlingStrategy()
> +{

OK

> scheduler.cpp:6843
>                  appendLogText(i18n("Warning: job '%1' guiding procedure failed, marking terminated due to errors.", currentJob->getName()));
> -                currentJob->setState(SchedulerJob::JOB_ERROR);
> +                currentJob->setState(SchedulerJob::JOB_ABORTED);
>  

OK! But change the log.

> scheduler.cpp:6980
>                  appendLogText(i18n("Warning: job '%1' focusing procedure failed, marking terminated due to errors.", currentJob->getName()));
> -                currentJob->setState(SchedulerJob::JOB_ERROR);
> +                currentJob->setState(SchedulerJob::JOB_ABORTED);
>  

OK! But change the log.

> scheduler.ui:1039
>            </property>
> -          <item row="0" column="0">
> -           <widget class="QCheckBox" name="altConstraintCheck">
> -            <property name="toolTip">
> -             <string>The object's altitude must remain equal or higher than the given value.</string>
> -            </property>
> +          <item row="0" column="2">
> +           <widget class="QLabel" name="label_10">

Sorry I didn't check the UI by importing the diff. Will do.

REPOSITORY
  R321 KStars

REVISION DETAIL
  https://phabricator.kde.org/D22446

To: wreissenberger, mutlaqja, TallFurryMan
Cc: kde-edu, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20190714/3bc6cfee/attachment-0001.html>