Background job class by ankitrox · Pull Request #507 · WordPress/performance

ankitrox · 2022-08-30T12:16:08Z

Summary

Background job class to manage the life cycle of a background job.

This class is responsible for:

Creation of job in the queue (taxonomy).
Locking/unlocking a particular job.
Job status management (running, failed, complete etc.)
Logging the errors for job.
Data retrieval of a job.

Fixes #503 #479

Relevant technical choices

Checklist

PR has either [Focus] or Infrastructure label.
PR has a [Type] label.
PR has a milestone or the no milestone label.

ankitrox · 2022-08-30T13:38:02Z

As of now, there is no update_data method as I did not find the real use case for updating the job data. In some edge case, consumer code can update the data using update_term_meta.
If such scenario to update data arises during development, we can add the method.

CC: @jjgrainger @felixarntz @eugene-manuilov

modules/images/regenerate-existing-images/background-process/class-perflab-background-job.php

ankitrox · 2022-08-30T17:01:10Z

@eugene-manuilov Thanks for review. Addressed the feedback, requesting re-review.

mukeshpanchal27

@ankitrox Left some reviews with suggestions.

Is a unit test remaining for these changes?

modules/images/regenerate-existing-images/background-process/class-perflab-background-job.php

…kground-job-class

ankitrox · 2022-09-01T14:13:01Z

@mukeshpanchal27 Added unit tests and addressed the feedback. Re-requesting review.

...modules/images/regenerate-existing-images/background-process/perflab-background-job-test.php

felixarntz · 2022-09-30T00:24:37Z

@ankitrox Left replies to the open comments. Once those iterations have been made, the PR should be good.

jjgrainger

Thanks @ankitrox we're almost there, just followed up on some comments from @felixarntz I think still need to be addressed.

jjgrainger · 2022-09-30T13:48:42Z

...modules/images/regenerate-existing-images/background-process/perflab-background-job-test.php

+		$this->assertEquals( 'perflab_job_action', constant( $job_class . '::META_KEY_JOB_ACTION' ) );
+		$this->assertEquals( 'perflab_job_data', constant( $job_class . '::META_KEY_JOB_DATA' ) );
+		$this->assertEquals( 'perflab_job_attempts', constant( $job_class . '::META_KEY_JOB_ATTEMPTS' ) );
+		$this->assertEquals( 'perflab_job_lock', constant( $job_class . '::META_KEY_JOB_LOCK' ) );
+		$this->assertEquals( 'perflab_job_errors', constant( $job_class . '::META_KEY_JOB_ERRORS' ) );
+		$this->assertEquals( 'perflab_job_status', constant( $job_class . '::META_KEY_JOB_STATUS' ) );
+		$this->assertEquals( 'perflab_job_completed_at', constant( $job_class . '::META_KEY_JOB_COMPLETED_AT' ) );
+		$this->assertEquals( 'queued', constant( $job_class . '::JOB_STATUS_QUEUED' ) );
+		$this->assertEquals( 'running', constant( $job_class . '::JOB_STATUS_RUNNING' ) );
+		$this->assertEquals( 'partial', constant( $job_class . '::JOB_STATUS_PARTIAL' ) );
+		$this->assertEquals( 'completed', constant( $job_class . '::JOB_STATUS_COMPLETE' ) );
+		$this->assertEquals( 'failed', constant( $job_class . '::JOB_STATUS_FAILED' ) );


@ankitrox I agree with @felixarntz here, lets keep it simple and reference the class name directly.

jjgrainger · 2022-09-30T13:54:08Z

...modules/images/regenerate-existing-images/background-process/perflab-background-job-test.php

+	 *
+	 * @var Perflab_Background_Job
+	 */
+	private $job;


@ankitrox have you addressed this?

...modules/images/regenerate-existing-images/background-process/perflab-background-job-test.php

felixarntz

@ankitrox A few last fixes needed here. Please make sure to address the open comments from previous reviews as well.

modules/images/regenerate-existing-images/background-process/class-perflab-background-job.php

modules/images/regenerate-existing-images/helper.php

felixarntz · 2022-09-30T16:58:45Z

...modules/images/regenerate-existing-images/background-process/perflab-background-job-test.php

+		$this->assertEquals( 'perflab_job_action', constant( $job_class . '::META_KEY_JOB_ACTION' ) );
+		$this->assertEquals( 'perflab_job_data', constant( $job_class . '::META_KEY_JOB_DATA' ) );
+		$this->assertEquals( 'perflab_job_attempts', constant( $job_class . '::META_KEY_JOB_ATTEMPTS' ) );
+		$this->assertEquals( 'perflab_job_lock', constant( $job_class . '::META_KEY_JOB_LOCK' ) );
+		$this->assertEquals( 'perflab_job_errors', constant( $job_class . '::META_KEY_JOB_ERRORS' ) );
+		$this->assertEquals( 'perflab_job_status', constant( $job_class . '::META_KEY_JOB_STATUS' ) );
+		$this->assertEquals( 'perflab_job_completed_at', constant( $job_class . '::META_KEY_JOB_COMPLETED_AT' ) );
+		$this->assertEquals( 'queued', constant( $job_class . '::JOB_STATUS_QUEUED' ) );
+		$this->assertEquals( 'running', constant( $job_class . '::JOB_STATUS_RUNNING' ) );
+		$this->assertEquals( 'partial', constant( $job_class . '::JOB_STATUS_PARTIAL' ) );
+		$this->assertEquals( 'completed', constant( $job_class . '::JOB_STATUS_COMPLETE' ) );
+		$this->assertEquals( 'failed', constant( $job_class . '::JOB_STATUS_FAILED' ) );


This still needs to be addressed.

...modules/images/regenerate-existing-images/background-process/perflab-background-job-test.php

felixarntz · 2022-09-30T17:04:56Z

...modules/images/regenerate-existing-images/background-process/perflab-background-job-test.php

+	public function test_create_job() {
+		$this->assertInstanceOf( Perflab_Background_Job::class, $this->job );
+	}


This should be removed as it's not testing any part of the class, but rather the perflab_create_background_job() function, which is already covered by the other test class's test_perflab_create_background_job() method.

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

…WordPress/performance into experiment/background-job-class

jjgrainger

Thanks @ankitrox I've requested a couple of changes here, can you take a look?

modules/images/regenerate-existing-images/helper.php

jjgrainger · 2022-10-03T15:43:48Z

...modules/images/regenerate-existing-images/background-process/perflab-background-job-test.php

+	 *
+	 * @var Perflab_Background_Job
+	 */
+	private $job;


@ankitrox please can you address or respond to this comment from Felix

No need to have a class property for jobs created in tests. WordPress cleans up pretty much any changes to the database after each tests. So you don't need this property, simply use a local variable per test method - and then you can also remove the unnecessary tear_down() method implementation.

Co-authored-by: Joe Grainger <904708+jjgrainger@users.noreply.github.com>

ankitrox · 2022-10-03T18:54:32Z

@ankitrox please can you address or respond to this comment from Felix

@jjgrainger Please refer this comment, according to this comment we will be using $this->job property and will be creating job instance in setUp method.

felixarntz

Thanks @ankitrox! LGTM, just one tiny nit-pick.

@jjgrainger Could you please re-review as well?

...modules/images/regenerate-existing-images/background-process/perflab-background-job-test.php

mukeshpanchal27

Thanks @ankitrox Left some minor update, not blocker.

modules/images/regenerate-existing-images/helper.php

...modules/images/regenerate-existing-images/background-process/perflab-background-job-test.php

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

jjgrainger

Thanks @ankitrox great work here, approved!

Ankit Gade added 2 commits August 30, 2022 16:23

Background job class

f44e14b

Replace class constant with self

f998a71

ankitrox added [Focus] Images no milestone PRs that do not have a defined milestone for release [Type] Epic A high-level project / epic that will encompass several sub-issues [Plugin] Regenerate Existing Images Issues for the Regenerate Existing Images module labels Aug 30, 2022

ankitrox self-assigned this Aug 30, 2022

ankitrox requested review from adamsilverstein, eugene-manuilov, felixarntz and mukeshpanchal27 August 30, 2022 12:26

ankitrox mentioned this pull request Aug 30, 2022

Background process runner #498

Closed

3 tasks

Job retries logic

2de0335

ankitrox marked this pull request as ready for review August 30, 2022 13:02

ankitrox requested a review from jjgrainger as a code owner August 30, 2022 13:02

Ankit Gade added 2 commits August 30, 2022 18:44

Update return value

656d219

Add more class constants for job meta

cc068f6

eugene-manuilov reviewed Aug 30, 2022

View reviewed changes

Job status class constants

8152d06

ankitrox requested a review from eugene-manuilov August 30, 2022 17:05

mukeshpanchal27 reviewed Aug 31, 2022

View reviewed changes

Ankit Gade added 5 commits September 1, 2022 12:53

fix formatting

65e452d

Merge branch 'feature/regenerate-existing-images' into experiment/bac…

ed49a7e

…kground-job-class

Change visibility of methods

7208197

Tests for constants

a12a90a

Tests: Job class methods

c921934

ankitrox requested a review from mukeshpanchal27 September 1, 2022 14:12

ankitrox requested a review from mukeshpanchal27 September 29, 2022 16:12

felixarntz reviewed Sep 30, 2022

View reviewed changes

...modules/images/regenerate-existing-images/background-process/perflab-background-job-test.php Outdated Show resolved Hide resolved

...modules/images/regenerate-existing-images/background-process/perflab-background-job-test.php Outdated Show resolved Hide resolved

Remove repititive assertions

600cf7e

ankitrox requested a review from felixarntz September 30, 2022 10:14

Return term name as action

b59d01b

jjgrainger requested changes Sep 30, 2022

View reviewed changes

felixarntz reviewed Sep 30, 2022

View reviewed changes

ankitrox and others added 5 commits October 3, 2022 12:52

tests: tear down

64189ce

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Remove redundant code

0556dfe

Merge branch 'experiment/background-job-class' of https://github.com/…

64f507e

…WordPress/performance into experiment/background-job-class

Fix: data provider in test

973405a

Test for job action

c401be5

ankitrox requested review from felixarntz and jjgrainger October 3, 2022 12:25

jjgrainger requested changes Oct 3, 2022

View reviewed changes

ankitrox and others added 2 commits October 4, 2022 00:14

combine operations for title sanitization

d7676cf

Co-authored-by: Joe Grainger <904708+jjgrainger@users.noreply.github.com>

Fix: syntax error

8df43a3

felixarntz approved these changes Oct 3, 2022

View reviewed changes

...modules/images/regenerate-existing-images/background-process/perflab-background-job-test.php Outdated Show resolved Hide resolved

felixarntz requested a review from jjgrainger October 3, 2022 19:06

mukeshpanchal27 approved these changes Oct 4, 2022

View reviewed changes

ankitrox and others added 3 commits October 4, 2022 14:12

Docblock updates

0b889b7

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

Tests: covers annotation

0f75293

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

Correction: method name for testing job status

48342ad

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

jjgrainger approved these changes Oct 4, 2022

View reviewed changes

jjgrainger merged commit 64e7cb3 into feature/regenerate-existing-images Oct 4, 2022

jjgrainger deleted the experiment/background-job-class branch October 4, 2022 09:26

This was referenced Oct 4, 2022

Create Background Processing Infrastructure: Create perflab_create_background_job function #479

Closed

Create Background Processing Infrastructure: Create Perflab_Background_Job Class #503

Closed

Conversation

ankitrox commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Relevant technical choices

Checklist

Uh oh!

ankitrox commented Aug 30, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ankitrox commented Aug 30, 2022

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ankitrox commented Sep 1, 2022

Uh oh!

Uh oh!

Uh oh!

felixarntz commented Sep 30, 2022

Uh oh!

jjgrainger left a comment

Choose a reason for hiding this comment

Uh oh!

jjgrainger Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

jjgrainger Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixarntz Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

felixarntz Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

jjgrainger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jjgrainger Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

ankitrox commented Oct 3, 2022

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jjgrainger left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

ankitrox commented Aug 30, 2022 •

edited

Loading