Support Schedule #62

peterwilsoncc · 2018-08-23T01:53:33Z

Adds support for the WP Cron API's named schedules.

This defines the latest database version Cavalcade uses.

rmccue

Looks on-track to me, just some small nits. (Also, no need to prefix your commits with an issue number.)

rmccue · 2018-08-23T01:58:54Z

inc/class-command.php

+	 * Upgrade to the latest database schema.
+	 */
+	public function upgrade( ) {
+		require_once __DIR__ . '/upgrade/namespace.php';


This should be loaded in the main plugin file, not just-in-time.

In a6736c1

rmccue · 2018-08-23T01:59:14Z

inc/class-command.php

+		require_once __DIR__ . '/upgrade/namespace.php';
+
+		if ( Upgrade\upgrade_database() ) {
+			\WP_CLI::success( 'Database version upgraded.' );


WP_CLI is already use'd, so doesn't need the \

In ddb0f12, I've left the existing instances in the file to avoid touching extra lines.

rmccue · 2018-08-23T02:00:56Z

inc/class-command.php

+			return;
+		}
+
+		\WP_CLI::error( 'Databse upgrade not required.' );


Database. Also, IMO this should be ::success instead, unless there's a good reason to fail. Given there's no needs-upgrade command, you otherwise can't know beforehand if this will fail or not.

Spellng corrected, switched to success in 9bf2bdb

rmccue · 2018-08-23T02:02:32Z

inc/class-job.php

@@ -12,6 +12,7 @@ class Job {
 	public $start;
 	public $nextrun;
 	public $interval;
+	public $schedule;


This should probably default to __fake_schedule or whatever we called it?

Done in 38c423c

Removes a typo and always returns them as successful to avoid failures that can’t be preemeted.

peterwilsoncc · 2018-08-23T05:38:51Z

Needs some additional logic for rescheduling tasks.

Currently, rescheduling an event with the same schedule results in a new entry in the jobs table as the calls to array_diff() see them as different.

joehoyle · 2018-08-23T15:29:40Z

inc/class-command.php

+	 * Upgrade to the latest database schema.
+	 */
+	public function upgrade( ) {
+		if ( Upgrade\upgrade_database() ) {


Can this be run automatically, or are we thinking we'll need to run this on every site manually?

I was thinking to run in manually on every site, in part to avoid any performance issues around running the ALTER query on large tables. I'm not sure if this is a real or imagined issue.

As it currently stands, there is a bug around rescheduling that I need to figure out. Depending on the approach, I may need to modify each row as part of the upgrade routine.

After some testing this morning, I did need to add a data upgrade routine in bc50540

The Job instance always has a schedule set.

peterwilsoncc · 2018-08-25T06:46:17Z

@rmccue I reckon this is ready for another look:

Still requires upgrade routine
it adds best guess data on upgrade
version one database schemas include best guess schedule name
connector uses fallback generated in the Job object

Needs some additional logic for rescheduling tasks.

The comment above was a misunderstanding on your author's behalf...

peterwilsoncc · 2018-08-29T21:36:12Z

@joehoyle Requesting a review from you on @rmccue's suggestion it get reviewed by the servers team.

joehoyle

This looks good to me. As I mentioned before, I think the upgrade path if a bit of a faff, but like you mentioned I'm not sure if there's much way around that. I think in our case we'll just need to have a ticket to manually run it across everything. At the minimum I have no issue with this PR going in as-is. Thanks for all the work!

peterwilsoncc · 2018-08-29T21:38:16Z

inc/upgrade/namespace.php

+
+	$schedules = Cavalcade\get_schedules_by_interval();
+
+	foreach ( $schedules as $interval => $name ) {


I've tested this PR on my site without populating the data and it looks like we can get away without doing so due to the fallback mechanism in the Job class.

rmccue · 2018-08-31T01:00:09Z

inc/upgrade/namespace.php

+
+	$query = "ALTER TABLE `{$wpdb->base_prefix}cavalcade_jobs`
+			  ADD `schedule` varchar(255) DEFAULT NULL
+			  AFTER `interval`";


I think, although I am uncertain, that adding new columns into the middle of the table causes horrific performance problems. IIRC, it has to create a temporary table, copy all records, then replace the real one, locking the table the whole time.

Trying to verify that now, but given we don't really care about the order, should we just add this to the end? IIRC, the performance is much better with that.

I've moved it to the end in 01ab379.

Placing the new column in the middle of the table may lead to a performance hit as the table is upgraded.

rmccue

Let's ship it.

peterwilsoncc added 9 commits August 23, 2018 10:47

#29: Add constant for database version.

d9e0c65

This defines the latest database version Cavalcade uses.

#29: Record the current DB version when creating tables.

a829056

#29: An upgrade routine for bumping database verions.

538c710

#29: WP-CLI command to run upgrade routine.

5b04e57

#29: Function to get the current DB version.

7068ec0

#29: Include schedule in job if after DB upgrade.

88dcb97

#29: Include schedule string when getting/setting recurring jobs.

c3270b6

#29: Store version as MU option rather than site option.

77cfe29

#29: Check schedule is not empty, it’s always set.

3a1ae5c

rmccue requested changes Aug 23, 2018

View reviewed changes

peterwilsoncc mentioned this pull request Aug 23, 2018

Add proper support for wp_get_schedule() #33

Closed

peterwilsoncc added 4 commits August 23, 2018 13:06

Remove JIT loading of upgrade functions.

a6736c1

Remove unnecessary slashes on WP_CLI calls.

ddb0f12

Fix no upgrade required messaging.

9bf2bdb

Removes a typo and always returns them as successful to avoid failures that can’t be preemeted.

Set default on the Job object for the schedule.

38c423c

joehoyle reviewed Aug 23, 2018

View reviewed changes

peterwilsoncc added 6 commits August 24, 2018 09:59

Function to index Cron schedule names by interval.

0213361

Include schedule name in DB upgrade.

bc50540

CS fixes.

0127be4

Allow getting schedule’s name by individual interval.

fdc4b7d

Get fallback schedule when generating instance.

11072cb

Remove fallback checking when generating array.

ba641fc

The Job instance always has a schedule set.

This was referenced Aug 26, 2018

Some basic unit tests #66

Merged

Fix/cavalcade peterwilsoncc/doitlive.peterwilson.cc#55

Merged

peterwilsoncc requested a review from joehoyle August 29, 2018 21:35

joehoyle approved these changes Aug 29, 2018

View reviewed changes

peterwilsoncc commented Aug 29, 2018

View reviewed changes

rmccue reviewed Aug 31, 2018

View reviewed changes

Move new schedule column to end of table.

01ab379

Placing the new column in the middle of the table may lead to a performance hit as the table is upgraded.

rmccue approved these changes Oct 23, 2018

View reviewed changes

rmccue merged commit 5991b54 into master Oct 23, 2018

rmccue deleted the 29-support-schedule branch October 23, 2018 01:50

rmccue mentioned this pull request Oct 23, 2018

Update Cavalcade to latest humanmade/hm-platform#107

Closed

peterwilsoncc mentioned this pull request Apr 14, 2019

wp_get_schedule() not supported, causes issues with Jetpack 4.4 #29

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Schedule #62

Support Schedule #62

peterwilsoncc commented Aug 23, 2018

rmccue left a comment

rmccue Aug 23, 2018

peterwilsoncc Aug 23, 2018

rmccue Aug 23, 2018

peterwilsoncc Aug 23, 2018

rmccue Aug 23, 2018

peterwilsoncc Aug 23, 2018

rmccue Aug 23, 2018

peterwilsoncc Aug 23, 2018

peterwilsoncc commented Aug 23, 2018

joehoyle Aug 23, 2018

peterwilsoncc Aug 23, 2018

peterwilsoncc Aug 24, 2018

peterwilsoncc commented Aug 25, 2018 •

edited

peterwilsoncc commented Aug 29, 2018

joehoyle left a comment

peterwilsoncc Aug 29, 2018

rmccue Aug 31, 2018

peterwilsoncc Aug 31, 2018

rmccue left a comment


		$schedules = Cavalcade\get_schedules_by_interval();

		foreach ( $schedules as $interval => $name ) {

Support Schedule #62

Support Schedule #62

Conversation

peterwilsoncc commented Aug 23, 2018

rmccue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterwilsoncc commented Aug 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterwilsoncc commented Aug 25, 2018 • edited

peterwilsoncc commented Aug 29, 2018

joehoyle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmccue left a comment

Choose a reason for hiding this comment

peterwilsoncc commented Aug 25, 2018 •

edited