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

BackgroundJob API too hard and confusing to use #24609

Closed
BernhardPosselt opened this issue May 12, 2016 · 3 comments
Closed

BackgroundJob API too hard and confusing to use #24609

BernhardPosselt opened this issue May 12, 2016 · 3 comments

Comments

@BernhardPosselt
Copy link
Contributor

BernhardPosselt commented May 12, 2016

The OCP\BackgroundJob API is deprecated and 9.1 features a nice declarative way for registering Background Jobs in the info.xml.

However the current IJob/IJoblist API is a mess and very hard to use:

  • The IJob Interface requires you to implement method calls which make no sense, are pure boilerplate and whose PHPdoc description is either missing or unclear (maybe because the authors did not know why it was there in the first place)
    • setId: what's the usecase for this?
    • getId: redundant, I should not have to implement that
    • setArgument: no idea why it's needed, it's not used in JobList
    • getArgument: same as setArgument
    • getLastRun: why do I need to implement this? Shouldn't this already be done by core?
    • setLastRun: same as getLastRun
    • execute: circular dependencies and logger should be injected via the constructor. The joblist should remove a job not a job should access the joblist to remove itself. This is a potential trap, since the user needs to take care of this by himself
  • The API has circular dependencies because responsibilities are mixed (e.g.: jobs remove themselves from a joblist)
  • Classes which actually take care of the boilerplate are private, so you can not extend them.

I propose the following solution: Every Background Job implements this interface:

namespace OCP\BackgroundJob;

interface ITask {
    /**
     * Runs the task
     */
    function execute();
}

That's it. So what about Timed and Regular Jobs? Pure configuration that I should not have to worry about.

Timed Job:

<background-jobs>
        <job id="name" interval="600">OCA\News\Cron\Updater</job>
        <job id="same-job" interval="300">OCA\News\Cron\Updater</job>
</background-jobs>

Queued Job:

<background-jobs>
        <job id="name">OCA\News\Cron\Updater</job>
</background-jobs>

How would one register or remove a cronjob programmatically? Use a service:

interface ITaskService {
    /**
     * @param $appId app's id
     * @param $taskId task id, use this to register the same task twice. Has
     *                     to be unique in combination with the app id
     * @param $class the class to run, has to implement ITask
     * @param int $interval optional, if given sleeps at least that many seconds
     *                      before the next execution
     */
    function registerTask($appId, $taskId, $class, $interval = 0);

    /**
     * Removes a task from the execution plan
     * @param $appId 
     * @param $taskId
     */
    function unregisterTask($appId, $taskId);

    /**
     * Returns the last time a task was executed
     * @param $appId app's id
     * @param $taskId task's id
     *
     * @return Date instance when the task was run the last time
     */
    function getLastRun($appId, $taskId);
}

@MorrisJobke @nickvergessen @DeepDiver1975 @icewind1991 @Xenopathic @PVince81 @LukasReschke

@MorrisJobke
Copy link
Contributor

How would one register or remove a cronjob programmatically? Use a service:

That one is already part of the IJobList.

interface ITask

This makes a lot of sense and would mostly be covered by extending the abstract class \OC\ BackgroundJob\Job, which is obviously private.

@nickvergessen
Copy link
Contributor

Sounds somewhat similar to https://github.com/owncloud/core/tree/master/lib/public/Command which are also stored as Jobs. Maybe we shouldn't bring up a third solution to the same problem?

@MorrisJobke
Copy link
Contributor

Sounds somewhat similar to https://github.com/owncloud/core/tree/master/lib/public/Command which are also stored as Jobs.

The Commands are one time jobs, the stuff in here is for repeating jobs (cronjobs), right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants