-
Notifications
You must be signed in to change notification settings - Fork 804
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
Jetpack Sync: Checksum performance optimizations for Meta Sync Module #41390
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 1 file.
|
|
||
$meta_objects = array(); | ||
|
||
foreach ( $conditionals as $where ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we extract the code inside the foreach to a method and call it inside the ifs of lines 80 and 87?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice suggestion! Done with 92e223e but I refactored a lot of things so we'll need to re-test everything now to be on the safe side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this to improve Sync's performance.
I really like the approach and appreciate the unit tests.
Added two comments to try and reduce the number of foreach loops for clarity. Let me know what you think :)
I tested too and it worked flawlessly, so the comments shouldn't be a blocker though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking care of the comment.
I tested again and thinks still work as expected. Furthermore, it was already noticeable that fetching time was lower than in trunk with my testing site which doesn't have a huge amount of data. Great job!
Really excited to see the impact.
During
meta
Fix Checksums, it's possible to callAutomattic\Jetpack\Sync\Modules\Metas::get_objects_by_id
with a config of max 1200 metas to fetch from the DB.Up to know we used to fetch each one of them via a separate DB query.
This PR refactors
get_objects_by_id
so that we fetch those via a single query.However, taking into account that the resulting DB query can be very long, we have incorporated a buffer logic that compares the resulting DB query length with a default max length to split the queries if needed.
Proposed changes:
Automattic\Jetpack\Sync\Modules\Module
: IntroduceMAX_DB_QUERY_LENGTH
class constant, which holds the maximum allowed DB query length.Automattic\Jetpack\Sync\Modules\Meta
: Refactorget_objects_by_id
to fetch all metas via a single DB queryAutomattic\Jetpack\Sync\Modules\Meta
: Introduceget_prepared_meta_object
private method, used both byget_object_by_id
andget_objecst_by_id
in order to prepare the meta results in the same expected format.Other information:
Jetpack product discussion
pf5801-1mi-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions:
woocommerce_order_itemmeta
so make sure you have WooCommerce plugin active and a few orders toopostmeta
table on your WPCOM Cache sitepostmeta
postmeta
table on your WPCOM Cache site is populated. Note the total count and some sample metaspostmeta
table on your WPCOM Cache site matches the total count from before and the sample metas you noted hold the same valuescommentmeta
andwoocommerce_order_itemmeta