-
Notifications
You must be signed in to change notification settings - Fork 10.8k
Adding OrderTableDatastore and Meta caching integration #46023
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
Conversation
Hi @naman03malhotra, Apart from reviewing the code changes, please make sure to review the testing instructions and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed. You can follow this guide to find out what good testing instructions should look like: |
Pausing review on this until I can come up with a better testing process. |
Renaming methods to invalidate_ rather than delete, marking as @internal
…actions into their own methods. Breaking these out should simplify future refactoring to possibly move DB and Cache layer interactions into their own classes and let the datastore delegate to those.
@kalessil, I pushed some refactoring around the datastore methods that were both accessing cache and the db. I broke these out into db specific methods and cache specific methods and left the merging/management of the data in the original method. Think this simplifies those and better defines where the abstraction/separation should go for future iterations. Because the current abstractions of the DB layer within the datastores uses inheritance rather than composition, the future iterations won't be easy to implement without further compatibility concerns and likely temporary additional complexity, which is why I'd like to address those later. |
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.
@prettyboymp: with the latest updates, it's much easier to grasp, thanks for the updates!
Also leaving some implementation-related hints, feel free to ignore them or let me know if I should turn geeky mode and do deeper implementation review.
plugins/woocommerce/src/Internal/DataStores/CustomMetaDataStore.php
Outdated
Show resolved
Hide resolved
// phpcs:enable | ||
|
||
foreach ( $meta_rows as $meta_row ) { | ||
if ( ! isset( $meta_data[ $meta_row->object_id ] ) ) { |
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.
Nit: we should probably array fill instead before the loop - that'd reduce cognitive load abit.
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 this. After re-reviewing this, filling the array ahead of time for the $object_ids works out better programmatically here as well. This made me notice that, though unlikely, an order with no meta data would result in no value set for the object_id. This would both mean it would always be a cache miss and cause some warnings in OrdersTableDataStore::init_order_record()
.
plugins/woocommerce/src/Internal/DataStores/Orders/OrdersTableDataStore.php
Outdated
Show resolved
Hide resolved
plugins/woocommerce/src/Internal/DataStores/Orders/OrdersTableDataStore.php
Show resolved
Hide resolved
plugins/woocommerce/src/Internal/DataStores/Orders/OrdersTableDataStore.php
Outdated
Show resolved
Hide resolved
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.
LGTM, thanks for updating the PR!
@prettyboymp, what'd be our next steps to push the caching forward?
* The use of datastore caching moves persistent data caching to the datastore. Order object caching then only | ||
* acts as request level caching as the `order_objects` cache group is set as non-persistent. | ||
*/ | ||
return 'order_objects'; |
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.
Not sure if this is a valid concern or not, but adding it here just in case. It appears that this object_type
value is used for more than just a cache key. In particular, it's also used for hook names. It seems like there could be backcompat issues here if we conditionally change this. But I'm not sure what the alternative would be.
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.
I'm going to update this docblock to remove the reference to hooks/options. The only built in CacheEngine
implementation is WPCacheEngine.php
which doesn't have any underlying hooks or option usage.
I think the compatibility concern is limited. It would require that an extension both overwrote the cache engine implementation in a way where it depended on the object type returned from the default cache implementation for some behavior but hard coded the previous default in hook usage, or vice versa.
@@ -1,17 +1,30 @@ | |||
<?php | |||
/** | |||
* OrdersTableDataStoreMeta class file. | |||
* | |||
* @phpcs:disable Universal.NamingConventions.NoReservedKeywordParameterNames.objectFound |
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.
This should be re-enabled sometime after the offending code lines.
…et_object_type() Adjust phpcs enabling/disabling of NoReservedKeywordParameterNames in OrdersTableDataStoreMeta
…46023) Adding a feature for caching of raw order data at the OrderDataStore level. Closes woocommerce#45550 With this feature enabled: * The cache group used by OrderCache is moved to `order_objects` and made non-persistent * The OrderTableDataStore and OrderTableDataStoreMeta now cache retrieved data. * Cache for individual order data is purged automatically after a write operation to the order. This is all feature gated behind the `woocommerce_hpos_datastore_caching_enabled` option and is currently under alpha testing - meaning that the option to enable this feature is only available if `WOOCOMMERCE_ENABLE_ALPHA_FEATURE_TESTING` is defined as true.
Submission Review Guidelines:
Changes proposed in this Pull Request:
Closes #45550.
Background:
The current OrderCaching works by attempting to cache the Order objects. However, the current implementation of
WC_Data::__sleep()/__wakeup()
only allow the ID to be serialized. The reason for this was to avoid serializing the datastore with a data object, but also prevents issues where complex hierarchies of objects can end up getting cached with the order object. The end result means that any unserialized WC_Data object has to be reinitialized completely, requiring DB queries to retrieve the data for re-hydration.This PR introduces the following that are disabled by default:
order_object
cache group that is set as non-persistent. We don't want to save the order objects in persistent storage and the time spent Instantiating a WC_Order object is small, but can add up. So non-persistent cache is used to avoid re-instantiating the same object over and over again during a single request.WOOCOMMERCE_ENABLE_ALPHA_FEATURE_TESTING
constant that can be used during initial alpha integration of new features/enhancements. When defined, the option to enable order data caching is available through the advanced features settings.Benefits:
Alternative Considered:
I looked at improving the serialization handling of WC_Data and it's child classes so that we could serialize all the relevant data with the entities without connected services, like the data stores. There were a couple issues with this:
First, PHP limitations meant that the serialization/deserialization would need to be pretty complex: PHP doesn't have a simple way to use the default serialization and just exclude certain properties. To properly restore an object, each class in the hierarchy would need to potentially deal with any private properties it defines. With extended classes from other plugins, those child classes would need to be properly modified for this to work or compatibility would break.
Second, there is already a bit of juggling going on around caching orders because the data stores have accessible public API's for making updates. Shifting the caching to them allows them to directly control the cache and update it when needed.
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
Unit tests have been updated to validate proper cache invalidation.
The following can be used to test the functionality of the OrdersTableDataStore caching. :
PR46023Tests.php
somewhere within theplugins/woocommerce/tests/php/src
diretory.--filter=PR46023Tests
Manual Testing
define( 'WOOCOMMERCE_ENABLE_ALPHA_FEATURE_TESTING', true );
Additional Automated testing:
OrderUtil:: custom_orders_table_datastore_cache_enabled()
to always returntrue
;