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.

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

Convert to custom table #89

Merged
merged 92 commits into from Mar 29, 2017
Merged

Convert to custom table #89

merged 92 commits into from Mar 29, 2017

Conversation

ethitter
Copy link
Collaborator

@ethitter ethitter commented Feb 17, 2017

By introducing a custom table with columns and indices specific to the plugin's needs, issues with duplicate events, race conditions that create excessive events, and stale caches are all alleviated. Additionally, by adding a number of utility functions, the plugin's internal consistency is greatly improved.

Some of the changes herein:

  • table is created, version stored in an option, mirroring Core's behaviour
  • table includes a unique constraint to prevent duplicate entries from being added
  • CLI to purge legacy CPT data is introduced
  • various classes no longer contain methods to retrieve events thanks to the introduction of flexible helper functions

Outstanding concerns:

  • table lacks sufficient indices; currently, they're mostly used to block duplicates, not for performance
  • table is created on first load, which could lead to a race condition of table creation; I'm not sure it's safe to restrict to is_admin(), though, as WP will keep trying to create entries

Fixes #24
Fixes #39
Fixes #63
Fixes #64
See #84
Fixes #85
Fixes #90
Fixes #91
Fixes #94

Moving out of posts, into a table with appropriate indices and unique key constraints
Updates all of the plugin's core classes to use the new event store, primarily through the helper functions I should've introduced in the first place.

Still need to update tests and CLI commands
@ethitter ethitter added the wip label Feb 17, 2017
@ethitter ethitter self-assigned this Feb 17, 2017
@ethitter ethitter changed the title WIP: Conver to custom table WIP: Convert to custom table Feb 17, 2017
Core resets the option before its tests, so we perform the equivalent. Addressing #90 will make the non-Core approach unnecessary.
When running events, lookups will be simplified with this already in the DB
…butes

This logic became spread across various classes during initial development, and it's time to consolidate.
Considerable cleanup and simplification along the way
`get_event()` method was removed earlier
@ethitter ethitter changed the title WIP: Convert to custom table Convert to custom table Feb 18, 2017
@ethitter
Copy link
Collaborator Author

@mjangda & @joshbetz - this is ready for another look. There's an open question around stampedes with the initial table creation, but I've addressed the remaining feedback and left a few thoughts on how to deal with the table.

ethitter and others added 16 commits March 17, 2017 19:08
When events are capped in this way, if they don't appear in the reconstructed array, they can be continually rescheduled.

With the introduction of the block on existing-event creation introduced in 61597bc, this approach should longer be necessary. The creation of duplicates is also greatly reduced by the table's unique key.

Fixes #94
…en it will run

Race conditions are a concern, so it'd be better not to create the table without some control over when it happens.
…alled site, create table in limited circumstances
PHPUnit doesn't load the plugin until after it's called `wp_install()`, which blocks normal table creation and disables the plugin.
Copy link
Member

@mjangda mjangda left a comment

Choose a reason for hiding this comment

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

Add a minor note about possible improvement on how namespaces are referenced in the code.

I still worry a bit about the dynamic table creation, but I think the protections we have in place now will probably be good enough.

remove_filter( 'schedule_event', '__return_false' );
add_filter( 'pre_option_cron', array( \Automattic\WP\Cron_Control\Events_Store::instance(), 'get_option' ) );
add_filter( 'pre_update_option_cron', array( \Automattic\WP\Cron_Control\Events_Store::instance(), 'update_option' ), 10, 2 );
add_filter( 'schedule_event', array( \Automattic\WP\Cron_Control\Events_Store::instance(), 'block_creation_if_job_exists' ) );
Copy link
Member

Choose a reason for hiding this comment

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

Minor: if we add use \Automattic\WP\Cron_Control\Events_Store to the top of this file, we can simplify the calls to this namespace:

add_filter( 'pre_update_option_cron', array( Events_Store::instance(), 'update_option' ), 10, 2 );
add_filter( 'schedule_event', array( Events_Store::instance(), 'block_creation_if_job_exists' ) );

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. I honestly don't know why I didn't use use anywhere in the tests or CLI classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants