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
EZP-20193: Spi persistence cache using Stash #198
Conversation
Tries to de couple persistence handlers, lazy load them, and in later commits cache them one buy one.
This could/should have been done as a seperate persistence implementation wrapping around legacy/persistence for cleaner code, however some more cleanup is needed on how the handlers are loaded using DIC before that becomes easy to do (for instance they can use aliases like repository).
Done: - Rename to eZ Publish to make space for additional future info - Only show loaded handlers once and count next to it - strip out array part of argument var_export for better readability
*/ | ||
public function testGetName() | ||
{ | ||
$this->assertEquals( 'PersistenceLogger', $this->logger->getName() ); |
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.
Use a class constant instead ?
Besides my several comments, looks pretty good. Good job @andrerom ! |
} | ||
else | ||
{ | ||
/** @var $urlAliasUrlLookupCache int */ |
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.
Hinting non-existent variable.
Why? - All other handlers are interfaces - There is no base functionality in it - I need to do this to be able to extend base handler stuff from all handler impl (cache in this case)
@@ -82,6 +82,10 @@ services: | |||
arguments: [@service_container] | |||
|
|||
ezpublish.api.persistence_handler: | |||
#To disable cache, switch alias to ezpublish.api.storage_engine | |||
alias: ezpublish.spi.persistence.cache |
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.
A semantic config for enabling/disabling cache would be nice 😃
So as far as I understood, only APC caching is working with StashBundle ? If so, we might have an issue since APC cache is not shared between workers in FastCGI/FPM mode. |
$location = $this->persistenceFactory->getLocationHandler()->create( $locationStruct ); | ||
|
||
$this->cache->getItem( 'location', $location->id )->set( $location ); | ||
$this->cache->clear( 'content', 'locations', $location->contentId ); |
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 does not take care of content locations cache with root key, set in loadLocationsByContent().
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.
It does afaik, Stash cache is hierarchical.
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.
Only problem being root key is not separate argument but concatenated to Content 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.
Correction, looked at the doc and you are indeed right.
Ephemeral is enabled under the hood if InMemory setting is enabled, as it is by default in combination with BlackHole. As for the other issues:
|
{ | ||
$this->logger->logCall( __METHOD__, array( 'type' => $identifier ) ); | ||
$type = $this->persistenceFactory->getContentTypeHandler()->loadByIdentifier( $identifier ); | ||
$cache->set( $type->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.
Loaded ContentType can be cached here as well.
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.
True, but it might already be cached, if not it will be fetched on next call.
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.
fixed in a244002
👍 |
Thanks, that is it then, to get QA to get access to this I will now merge. The remaining (non blocking) open topics will be handled separately when time permits:
Other remaining issues left for future versions are written in the description of this PR. |
EZP-20193: Spi persistence cache using Stash
Changes Unknown when pulling 6254c8f on spiPersistenceCache into * on master*. |
Looks like a83c3ec for Symfony 5.2 was applied by accident on the Kernel branch 1.2 which supports Symfony 5.1.x.
This PR adds Persistence cache (aka "StorageEngine" cache) as a layer above the storage engine using a caching library called Stash which supports both APC, memcache(d) and others.
Note: Related to ezsystems/ezpublish-community#42
The benefit of doing this caching at this layer is that you don't have to care about permissions and can easily have one big pool of cache for all users, witch makes it much more memory efficient.
TODO:
TODO's for another time:
* This has the benefit of solving our issue with database connection being openend when repository is loaded. .