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

Use custom SQL instead of transient for queue lock #5239

Merged
merged 6 commits into from Oct 3, 2016
Merged

Use custom SQL instead of transient for queue lock #5239

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cf85651
Use custom SQL instead of transient for queue lock
gravityrail Oct 2, 2016
0ddc717
Move queue timeout setting into sync settings file; small bugfix
gravityrail Oct 3, 2016
4b90766
Optimise queries for minimal table fragmentation, and reduce lock tim…
gravityrail Oct 3, 2016
95494e7
Make lock timeout 2 mins just to be safe, since execution timeout is …
gravityrail Oct 3, 2016
6ce5ef0
Explicitly set autoload to false when creating queue lock option
gravityrail Oct 3, 2016
c09ff18
Fix syntax error on PHP < 5.6
gravityrail Oct 3, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions sync/class.jetpack-sync-defaults.php
View file
Open in desktop
Expand Up @@ -288,4 +288,5 @@ static function get_max_sync_execution_time() {
static $default_render_filtered_content = 1; // render post_filtered_content
static $default_sync_callables_wait_time = MINUTE_IN_SECONDS; // seconds before sending callables again
static $default_sync_constants_wait_time = HOUR_IN_SECONDS; // seconds before sending constants again
static $default_sync_queue_lock_timeout = MINUTE_IN_SECONDS;
}
52 changes: 48 additions & 4 deletions sync/class.jetpack-sync-queue.php
View file
Open in desktop
Expand Up @@ -325,18 +325,62 @@ function unlock() {
}

private function get_checkout_id() {
return get_transient( $this->get_checkout_transient_name() );
global $wpdb;
$checkout_value = $wpdb->get_var(
$wpdb->prepare(
"SELECT option_value FROM $wpdb->options WHERE option_name = %s",
$this->get_lock_option_name()
)
);

if ( $checkout_value ) {
list( $checkout_id, $timestamp ) = explode( ':', $checkout_value );
if ( intval( $timestamp ) > time() ) {
return $checkout_id;
}
}

return false;
}

private function set_checkout_id( $checkout_id ) {
return set_transient( $this->get_checkout_transient_name(), $checkout_id, 5 * 60 ); // 5 minute timeout
global $wpdb;

$expires = time() + Jetpack_Sync_Defaults::$default_sync_queue_lock_timeout;
$updated_num = $wpdb->query(
$wpdb->prepare(
"UPDATE $wpdb->options SET option_value = %s WHERE option_name = %s",
"$checkout_id:$expires",
$this->get_lock_option_name()
)
);

if ( ! $updated_num ) {
$updated_num = $wpdb->query(
$wpdb->prepare(
"INSERT INTO $wpdb->options ( option_name, option_value ) VALUES ( %s, %s )",
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 try to INSERT first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed in Slack, updating in place and never deleting is a better strategy for MySQL performance (we think) so going to leave this ordering as-is.

$this->get_lock_option_name(),
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 we should set the option to not be auto loaded here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh damn! Great catch.

"$checkout_id:$expires"
)
);
}

return $updated_num;
}

private function delete_checkout_id() {
delete_transient( $this->get_checkout_transient_name() );
global $wpdb;
// rather than delete, which causes fragmentation, we update in place
$wpdb->query(
Copy link
Member

@enejb enejb Oct 3, 2016 •
edited

Choose a reason for hiding this comment

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

Why not just delete the value from the DB?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it turns out updates in place are typically faster than inserts+deletes in MySQL, if you expect to be doing them a lot.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense :)

$wpdb->prepare(
"UPDATE $wpdb->options SET option_value = %s WHERE option_name = %s",
"0:0",
$this->get_lock_option_name()
)
);
}

private function get_checkout_transient_name() {
private function get_lock_option_name() {
return "jpsq_{$this->id}_checkout";
}

Expand Down