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.

Sign up for GitHub

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

Jump to bottom

Change cron schedule to 10 mins, and stagger the jobs #5879

Merged
merged 10 commits into from Jan 6, 2017
Merged

Change cron schedule to 10 mins, and stagger the jobs #5879

merged 10 commits into from Jan 6, 2017

Conversation

gravityrail
Copy link
Contributor

For large multisites with reliable cron running (e.g. Cavalcade), you can end up with hundreds of jobs running simultaneously because they're scheduled close together.

This change spaces the jobs out to 10 minutes (from 5) and randomises the start time so that they don't all start at once.

@gravityrail gravityrail self-assigned this Dec 14, 2016
@gravityrail gravityrail added [Package] Sync [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Team] Poseidon labels Dec 14, 2016
@gravityrail gravityrail added this to the 4.5 milestone Dec 14, 2016
ebinnion
ebinnion reviewed Dec 15, 2016
View reviewed changes
sync/class.jetpack-sync-actions.php
@@ -296,23 +296,23 @@ static function maybe_schedule_sync_cron( $schedule, $hook ) {
}
$schedule = self::sanitize_filtered_sync_cron_schedule( $schedule );

// stagged the schedule time by some random fraction of the schedule interval
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"stagged" should be "Stagger".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we/could we add a test to make sure that the scheduling is random?

Also, as is, I don't think this would update plugins that exist with the old schedule. To clear out the old schedule jobs, we may need to add a condition here to check if we're upgrading from less than 4.5, and then clear out the scheduled jobs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we/could we add a test to make sure that the scheduling is random?

If we added such a test, we'd just be testing mt_rand (external function) rather than anything internal. I choose to trust that mt_rand is random :)

Also, as is, I don't think this would update plugins that exist with the old schedule.

Actually, it does check if the hook is already scheduled, and then un-schedules and re-schedules it - see below:

if ( $schedule != wp_get_schedule( $hook ) ) { //...

lezama
lezama reviewed Dec 15, 2016
View reviewed changes
sync/class.jetpack-sync-actions.php
@@ -296,23 +296,23 @@ static function maybe_schedule_sync_cron( $schedule, $hook ) {
}
$schedule = self::sanitize_filtered_sync_cron_schedule( $schedule );

// stagged the schedule time by some random fraction of the schedule interval
// so that on multisites with strong scheduling we don't DOS ourselves.
$schedule_time = time() + rand( 0, self::DEFAULT_SYNC_CRON_INTERVAL_VALUE );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we start rand at self::DEFAULT_SYNC_CRON_INTERVAL_VALUE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

enejb
enejb reviewed Dec 16, 2016
View reviewed changes
Copy link
Member

@enejb enejb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if instead of using a random number could we use blog_id to schedule the job.
Since they are unique across the network. And we could have a mini interval but it would be spaced out more if the user has a lot of sites on the network.

sync/class.jetpack-sync-actions.php
@@ -54,7 +54,7 @@ static function init() {
self::initialize_listener();
}

add_action( 'init', array( __CLASS__, 'add_sender_shutdown' ), 90 );
add_action( 'plugins_loaded', array( __CLASS__, 'add_sender_shutdown' ), 90 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain how this fixes the fatal error?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be removed.

sync/class.jetpack-sync-actions.php
@@ -10,7 +10,7 @@ class Jetpack_Sync_Actions {
static $sender = null;
static $listener = null;
const DEFAULT_SYNC_CRON_INTERVAL_NAME = 'jetpack_sync_interval';
const DEFAULT_SYNC_CRON_INTERVAL_VALUE = 300; // 5 * MINUTE_IN_SECONDS;
const DEFAULT_SYNC_CRON_INTERVAL_VALUE = 600; // 10 * MINUTE_IN_SECONDS;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change this value just for multisites?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure if we should change it at all... we don't have great data on this :(

Copy link
Contributor

@ebinnion ebinnion Jan 4, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure if we should change it at all... we don't have great data on this :(

Although, if we do change it, I'd prefer the default to be more conservative and allow sites the ability to increase it.

ebinnion
ebinnion reviewed Jan 4, 2017
View reviewed changes
sync/class.jetpack-sync-actions.php
@@ -296,23 +296,23 @@ static function maybe_schedule_sync_cron( $schedule, $hook ) {
}
$schedule = self::sanitize_filtered_sync_cron_schedule( $schedule );

// stagger the schedule time by some random fraction of the 2 x schedule interval (somewhat arbitrarily)
// so that on multisites with strong scheduling we don't DOS ourselves.
$schedule_time = time() + mt_rand( 0, 2*self::DEFAULT_SYNC_CRON_INTERVAL_VALUE );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably add space on either side of *.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took me a second to grok this. Basically, either schedule the event to start right away or up to 20 minutes from now.

@ebinnion
Copy link
Contributor

ebinnion commented Jan 4, 2017 •
edited

Unless I'm misunderstanding, this new interval will not be applied to new sites since the cron job would have already been scheduled and we're not changing the schedule name in DEFAULT_SYNC_CRON_INTERVAL_NAME.

So, we'll probably need to add something in cleanup_on_upgrade that clears out the old scheduled jobs.

@ebinnion ebinnion added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Jan 4, 2017
@ebinnion ebinnion force-pushed the fix/only-run-cron-on-large-queue branch from d8cc5e2 to 1863c15 Compare January 6, 2017 15:01
@ebinnion ebinnion added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 6, 2017
@ebinnion ebinnion force-pushed the fix/only-run-cron-on-large-queue branch from 91b38bc to d4b7485 Compare January 6, 2017 17:38
@enejb
Copy link
Member

enejb commented Jan 6, 2017

Let's merge this after the tests pass 👍

@ebinnion ebinnion merged commit 0e89273 into master Jan 6, 2017
@ebinnion ebinnion deleted the fix/only-run-cron-on-large-queue branch January 6, 2017 18:28
@ebinnion ebinnion removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Jan 6, 2017
@dereksmart dereksmart restored the fix/only-run-cron-on-large-queue branch January 6, 2017 18:36
@dereksmart
Copy link
Member

Merged to 4.5 e20b288

@dereksmart dereksmart deleted the fix/only-run-cron-on-large-queue branch January 6, 2017 18:38
@enejb enejb mentioned this pull request Jan 6, 2017
Closed
jeherve added a commit that referenced this pull request Jan 9, 2017
@jeherve
Changelog: add #5879
366bc5b
dereksmart pushed a commit that referenced this pull request Jan 17, 2017
@jeherve @dereksmart
Changelog: add 4.4.2 release post link.
711efa1 
CHangelog: add #5457

Changelog: add #5487

Changelog: add #5708

Changelog: add #5879

Changelog: add #5932

Changelog: add #5963

Changelog: add #5968

Changelog: add #5996

Changelog: add #5998

Changelog: add #5999

Changelog: add #6012

Changelog: add #6013

Changelog: add #6014

Changelog: add #6015

Changelog: add #6023

Changelog: add #6024

Changelog: add #6030

Changelog: add #5465

CHangelog: add #6063

Changelog: add #6025

Changelog: add #5974

Changelog: add #6059

Changelog: add #6046

Changelog: add #5418

Changelog: move things around and add missing information.

Changelog: add #5565

Changelog: add #6087

Changelog: add #6095
dereksmart pushed a commit that referenced this pull request Jan 17, 2017
@jeherve @dereksmart
Changelog: add #5866
10bc6c3 
Changelog: add #5867

Changelog: add #5874

Changelog: add #5905

Changelog: add #5906

Changelog: add #5931

Changelog: add #5933

Changelog: add #5934

Bring over 4.4.2 changelog from branch-4.4

@see 18012a3

Changelog: add #5976, #5978, #5983

Changelog: add #5917

Changelog: add #5832

Changelog: add 4.4.2 release post link.

CHangelog: add #5457

Changelog: add #5487

Changelog: add #5708

Changelog: add #5879

Changelog: add #5932

Changelog: add #5963

Changelog: add #5968

Changelog: add #5996

Changelog: add #5998

Changelog: add #5999

Changelog: add #6012

Changelog: add #6013

Changelog: add #6014

Changelog: add #6015

Changelog: add #6023

Changelog: add #6024

Changelog: add #6030

Changelog: add #5465

CHangelog: add #6063

Changelog: add #6025

Changelog: add #5974

Changelog: add #6059

Changelog: add #6046

Changelog: add #5418

Changelog: move things around and add missing information.

Changelog: add #5565

Changelog: add #6087

Changelog: add #6095

Readme: add @tyxla to the list of contributors.

Improved changelog for your readability and enjoyment

updated the release date

finalizing the changelog with a few more edits
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lezama lezama lezama left review comments

@enejb enejb enejb left review comments

@ebinnion ebinnion ebinnion left review comments

Assignees

@gravityrail gravityrail

Labels
[Package] Sync
Projects
None yet
Milestone
4.5
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@gravityrail @ebinnion @enejb @dereksmart @lezama @jeherve