Adding background:queue commands: status and delete #31630

DeepDiver1975 · 2018-06-04T12:45:48Z

Description

Adding two new commands for listing background jobs and deleting individual jobs

Related Issue

#31617

Motivation and Context

Adding a full feature set around occ and background jobs

How Has This Been Tested?

manual testing
unit tests

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

tomneedham · 2018-06-04T13:18:32Z

core/Command/Background/Queue/Delete.php

@@ -0,0 +1,45 @@
+<?php
+


missing copyright header

tomneedham · 2018-06-04T13:18:59Z

core/Command/Background/Queue/Delete.php

+		$job = $this->jobList->getById($id);
+		if ($job === null) {
+			$output->writeln("Job with id <$id> is not known.");
+			return;


return 1 for the error?

tomneedham · 2018-06-04T13:19:06Z

core/Command/Background/Queue/Delete.php

+		}
+
+		$this->jobList->removeById($id);
+		$output->writeln('Job has been deleted.');


tomneedham · 2018-06-04T13:20:15Z

lib/public/BackgroundJob/IJobList.php

+	 * iterate over all jobs in the queue
+	 *
+	 * @return void
+	 * @since 10.0.3


codecov · 2018-06-04T13:26:12Z

Codecov Report

Merging #31630 into master will increase coverage by <.01%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##             master   #31630      +/-   ##
============================================
+ Coverage     62.88%   62.88%   +<.01%     
- Complexity    18381    18393      +12     
============================================
  Files          1151     1153       +2     
  Lines         69111    69151      +40     
  Branches       1260     1260              
============================================
+ Hits          43457    43485      +28     
- Misses        25285    25297      +12     
  Partials        369      369

Flag	Coverage Δ	Complexity Δ
#javascript	`52.39% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`64.09% <66.66%> (ø)`	`18393 <13> (+12)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
lib/private/BackgroundJob/JobList.php	`74% <0%> (-5.86%)`	`33 <5> (+4)`
core/register_command.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
core/Command/Background/Queue/Delete.php	`100% <100%> (ø)`	`4 <4> (?)`
core/Command/Background/Queue/Status.php	`100% <100%> (ø)`	`4 <4> (?)`
lib/private/BackgroundJob/Legacy/RegularJob.php	`0% <0%> (ø)`	`3% <0%> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a308c24...1a99e5d. Read the comment docs.

mmattel · 2018-06-04T13:39:36Z

For the sake of documentation 😄

background:queue:delete 'id'
background:queue:status 'Id', 'Job', 'Last run', 'Arguments'

mmattel · 2018-06-04T13:54:36Z

@DeepDiver1975
in command delete, you have id
in command status, you have Id
could you haronize them eg to ID (or whatever but equal) ?

in command status, you have Arguments, I guess this means Job Arguments or ?
if yes, what about renaming to make it clear which arguments are meant, else you could think of occ command arguments

@tomneedham you have in #31617 in code TimedJob.php line 59 id. Please sync with @DeepDiver1975 and have everywhere the same

@DeepDiver1975 , same codline as above in TimedJob.php @tomneedham names a class here as additional text printed. Either this is missing in your PR in status or it maybe equal to your Arguments value. Can you pls sync and harmonize?

Edited, falsly used @butonic instead of @tomneedham , pls excuse

mmattel · 2018-06-04T14:45:04Z

@DeepDiver1975
Having a background:queue:delete command.
What happens if I delete all jobs or accidentially one job?
How can I come back to a working state?
What about a background:queue:reset command which resets all jobs to a state like the system got shipped?

DeepDiver1975 · 2018-06-04T21:06:13Z

in command delete, you have id
in command status, you have Id

I see no inconsistency here - id in the argument follows the command argument naming convention and the Id in the status command is a human readable table column - all columns are upper case.

in command status, you have Arguments, I guess this means Job Arguments or ?
if yes, what about renaming to make it clear which arguments are meant, else you could think of occ command arguments

agreed

@DeepDiver1975 , same codline as above in TimedJob.php @tomneedham names a class here as additional text printed. Either this is missing in your PR in status or it maybe equal to your Arguments value. Can you pls sync and harmonize?

job arguments hold the class name as well as required arguments for the execution of the job.
the timedjob is already an instanciated job of one specific type.

the information is identical but it's represenations is different - I see no way to harmonize this right away

What happens if I delete all jobs or accidentially one job?
How can I come back to a working state?

well - this is what backups are for ;-)
seriously: one better knows what he does when killing a job - we can think about a enqueue commend or something

reset is not the way to go because there are multiple sources how jobs are enqueued - we can only restore some of them. I dislike partial solutions

mmattel · 2018-06-05T08:11:19Z

@DeepDiver1975 @tomneedham
delete: ->addArgument('id', InputArgument::REQUIRED, 'id of the job to be deleted');
status: $t->setHeaders(['Id', 'Job', 'Last run', 'Job Arguments']);
Execute:->addArgument('jobId', InputArgument::REQUIRED)

All three lines are printed and the user can see it.
The presentation to the user is different id vs Id vs jobId.
Independent for what you decide, the look should be consistent else this is unprofessional.
Same version of how we write it should be taken in all PR´s.

job arguments hold the class name as well as required arguments for the execution of the job

Could we agree to have the same naming in all PR´s for this too, like Job Arguments or what ever

one better knows what he does when killing a job

I would not sign this... I do not know, but a possible solution could be to have a marker in the database for jobs defined by core and you can enable/disable them. All other jobs not beeing part of core do not get/have that marker.

I dislike partial solutions

Agreed - therefore I take the effort to write my thoughts down 😄

DeepDiver1975 · 2018-06-05T08:52:41Z

Execute:->addArgument('jobId', InputArgument::REQUIRED)

I agree to use the same argument name here.

tomneedham · 2018-06-05T09:34:13Z

Status and delete are useful commands, but only for debugging / analysis, not normal operation. This should be clear in any documentation.

Likewise with my execute command, it is only really for use in exceptional scenarios where we need to forcefully trigger a job on demand for analysis. It should not be expected that these commands are used in normal operation.

tomneedham · 2018-06-05T09:35:03Z

Need to update tests with expected table output

DeepDiver1975 · 2018-06-05T09:57:56Z

Need to update tests with expected table output

addressed

mmattel · 2018-06-05T11:15:18Z

@DeepDiver1975
being a pain, I know, but id -> Job ID
delete: Line 44, 57
status: Line 54

(all changes including the proposal below need table length adoption ...)

@tomneedham I do not know where the right place to note that is so I do here,
In your PR in Job.php in line 59 and 74 you define a log output having the name class and arguments
@DeepDiver1975 in status beginning with line 54 you define the printable output

Looking at both PR´s I see that \get_class gets called but in job.php it is printed as class while in status it is printed as Job.
I propose to have on both the term Job Name and for arguments the term Job Arguments

tomneedham · 2018-06-05T12:54:50Z

@mmattel thx, known - waiting for this then I will rebase based on feedback here

DeepDiver1975 · 2018-06-11T19:46:55Z

we can harmonize print out later on ...

DeepDiver1975 · 2018-06-11T19:47:09Z

@tomneedham feel free to adjust your PR - THX

PVince81 · 2018-06-12T16:00:35Z

@DeepDiver1975 backport to stable10 ?

DeepDiver1975 · 2018-06-13T06:56:16Z

we will backport once #31617 is merged and we backport this together as one

phil-davis · 2019-03-18T05:26:27Z

Backport stable10 is in PR #34783
Passes CI

PVince81 · 2019-03-18T14:28:34Z

some objections to be clarified in #31617 first

cc @tomneedham

phil-davis · 2019-03-29T12:09:10Z

I added label only-in-master so I can find and recognise this PR easily.
If/when the backporting is merged:

remove label only-in-master
remove label backport-request

phil-davis · 2019-04-03T04:51:00Z

some objections to be clarified in #31617 first

cc @tomneedham

Ping @tomneedham or anyone for comment about backporting.

DeepDiver1975 added the 3 - To Review label Jun 4, 2018

DeepDiver1975 added this to the development milestone Jun 4, 2018

DeepDiver1975 self-assigned this Jun 4, 2018

DeepDiver1975 requested review from tomneedham and mmattel June 4, 2018 12:45

tomneedham suggested changes Jun 4, 2018

View reviewed changes

DeepDiver1975 force-pushed the feature/queue-status-and-queue-delete branch from d8f890e to d11d73e Compare June 4, 2018 13:26

DeepDiver1975 added the backport-request label Jun 4, 2018

tomneedham approved these changes Jun 4, 2018

View reviewed changes

mmattel mentioned this pull request Jun 4, 2018

Add background:execute occ command for running cron jobs manually #31617

Merged

9 tasks

DeepDiver1975 force-pushed the feature/queue-status-and-queue-delete branch from d11d73e to c8971ee Compare June 4, 2018 21:08

Adding background:queue commands: status and delete

1a99e5d

DeepDiver1975 force-pushed the feature/queue-status-and-queue-delete branch from c8971ee to 1a99e5d Compare June 5, 2018 09:57

DeepDiver1975 merged commit 5b9c82b into master Jun 11, 2018

DeepDiver1975 deleted the feature/queue-status-and-queue-delete branch June 11, 2018 19:46

mmattel mentioned this pull request Jun 12, 2018

Harmonize printout: background:queue/execute commands #31727

Closed

mmattel mentioned this pull request Jun 12, 2018

occ commands to manage the job list #24551

Closed

4 tasks

mmattel mentioned this pull request Jun 13, 2018

Adding command background:queue:add #31743

Closed

PVince81 modified the milestones: development, QA Jun 13, 2018

settermjd mentioned this pull request Jun 14, 2018

Document: Adding background:queue commands: status and delete owncloud-archive/documentation#4207

Merged

mmattel mentioned this pull request Jul 9, 2018

Remove CleanupChunks background job owncloud-archive/documentation#3799

Merged

This was referenced Nov 19, 2018

Backport reminder for #4207, waiting core/stable10 to be merged owncloud/docs#266

Closed

Improvement: Remove CleanupChunks background job owncloud/docs#273

Closed

Document: Adding background:queue commands: status and delete owncloud/docs#287

Closed

phil-davis mentioned this pull request Mar 18, 2019

[stable10] Adding background:queue commands: status and delete #34783

Merged

phil-davis mentioned this pull request Mar 19, 2019

[stable10] Log exception when background job class not found #34723

Merged

phil-davis added the only-in-master label Mar 29, 2019

PVince81 removed backport-request labels Apr 12, 2019

paurakhsharma mentioned this pull request May 15, 2019

write cli test for background:queue status and delete #35229

Closed

lock bot locked as resolved and limited conversation to collaborators Apr 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding background:queue commands: status and delete #31630

Adding background:queue commands: status and delete #31630

DeepDiver1975 commented Jun 4, 2018 •

edited

Loading

tomneedham Jun 4, 2018

tomneedham Jun 4, 2018

tomneedham Jun 4, 2018

tomneedham Jun 4, 2018

codecov bot commented Jun 4, 2018 •

edited

Loading

mmattel commented Jun 4, 2018

mmattel commented Jun 4, 2018 •

edited

Loading

mmattel commented Jun 4, 2018

DeepDiver1975 commented Jun 4, 2018

mmattel commented Jun 5, 2018

DeepDiver1975 commented Jun 5, 2018

tomneedham commented Jun 5, 2018

tomneedham commented Jun 5, 2018

DeepDiver1975 commented Jun 5, 2018

mmattel commented Jun 5, 2018 •

edited

Loading

tomneedham commented Jun 5, 2018

DeepDiver1975 commented Jun 11, 2018

DeepDiver1975 commented Jun 11, 2018

PVince81 commented Jun 12, 2018

DeepDiver1975 commented Jun 13, 2018

phil-davis commented Mar 18, 2019

PVince81 commented Mar 18, 2019

phil-davis commented Mar 29, 2019 •

edited by PVince81

Loading

phil-davis commented Apr 3, 2019

Adding background:queue commands: status and delete #31630

Adding background:queue commands: status and delete #31630

Conversation

DeepDiver1975 commented Jun 4, 2018 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

tomneedham Jun 4, 2018

Choose a reason for hiding this comment

tomneedham Jun 4, 2018

Choose a reason for hiding this comment

tomneedham Jun 4, 2018

Choose a reason for hiding this comment

tomneedham Jun 4, 2018

Choose a reason for hiding this comment

codecov bot commented Jun 4, 2018 • edited Loading

Codecov Report

mmattel commented Jun 4, 2018

mmattel commented Jun 4, 2018 • edited Loading

mmattel commented Jun 4, 2018

DeepDiver1975 commented Jun 4, 2018

mmattel commented Jun 5, 2018

DeepDiver1975 commented Jun 5, 2018

tomneedham commented Jun 5, 2018

tomneedham commented Jun 5, 2018

DeepDiver1975 commented Jun 5, 2018

mmattel commented Jun 5, 2018 • edited Loading

tomneedham commented Jun 5, 2018

DeepDiver1975 commented Jun 11, 2018

DeepDiver1975 commented Jun 11, 2018

PVince81 commented Jun 12, 2018

DeepDiver1975 commented Jun 13, 2018

phil-davis commented Mar 18, 2019

PVince81 commented Mar 18, 2019

phil-davis commented Mar 29, 2019 • edited by PVince81 Loading

phil-davis commented Apr 3, 2019

DeepDiver1975 commented Jun 4, 2018 •

edited

Loading

codecov bot commented Jun 4, 2018 •

edited

Loading

mmattel commented Jun 4, 2018 •

edited

Loading

mmattel commented Jun 5, 2018 •

edited

Loading

phil-davis commented Mar 29, 2019 •

edited by PVince81

Loading