FEATURE: Overhaul ContentCacheFlusher #5221
Conversation
| public function nodeTagForIdentifier(string $identifier, Node $contextNode): array | ||
| { | ||
| return CacheTag::forNodeAggregate( | ||
| $contextNode->contentRepositoryId, | ||
| $contextNode->workspaceName, | ||
| NodeAggregateId::fromString($identifier) | ||
| )->value; | ||
| return [ | ||
| CacheTag::forNodeAggregate( | ||
| $contextNode->contentRepositoryId, | ||
| $contextNode->workspaceName, | ||
| NodeAggregateId::fromString($identifier) | ||
| )->value, | ||
| CacheTag::forNodeAggregate( | ||
| $contextNode->contentRepositoryId, | ||
| CacheTagWorkspaceName::ANY, | ||
| NodeAggregateId::fromString($identifier) | ||
| )->value, | ||
| CacheTag::forWorkspaceName( | ||
| $contextNode->contentRepositoryId, | ||
| $contextNode->workspaceName | ||
| )->value | ||
| ]; |
There was a problem hiding this comment.
I have to change the return type here, to be able to generate also the workspace tags for identifiers, which is breaking if someone relies on a string. But it works without any changes in the @cache.entryTags
Neos.Neos/Classes/Fusion/Cache/GraphProjectorCatchUpHookForCacheFlushing.php
Show resolved
Hide resolved
| public function onBeforeBatchCompleted(): void | ||
| { | ||
| foreach ($this->flushNodeAggregateRequestsOnBeforeBatchCompleted as $index => $request) { | ||
| $this->contentCacheFlusher->flushNodeAggregate($request, CacheFlushingStrategy::IMMEDIATELY); | ||
| $this->flushNodeAggregateRequestsOnAfterCatchUp[$index] = $request; | ||
| } | ||
| $this->flushNodeAggregateRequestsOnBeforeBatchCompleted = []; | ||
|
|
||
| foreach ($this->flushWorkspaceRequestsOnBeforeBatchCompleted as $index => $request) { | ||
| $this->contentCacheFlusher->flushWorkspace($request, CacheFlushingStrategy::IMMEDIATELY); | ||
| $this->flushWorkspaceRequestsOnAfterCatchUp[$index] = $request; | ||
| } | ||
| $this->flushWorkspaceRequestsOnBeforeBatchCompleted = []; | ||
| } |
There was a problem hiding this comment.
We might need to discuss this "double-cache-flush" approach. See Sebastians description in the head of this file (eventual consistency) vs. performance considerations.
| // Prevent infinite loops | ||
| if (!$collectedParentNodeAggregateIds->contain($parentNodeAggregateId)) { | ||
| $collectedParentNodeAggregateIds = $collectedParentNodeAggregateIds->merge(NodeAggregateIds::create($parentNodeAggregateId)); | ||
| $collectedParentNodeAggregateIds = $this->determineParentNodeAggregateIds($workspaceName, $parentNodeAggregateId, $collectedParentNodeAggregateIds); |
There was a problem hiding this comment.
We need to prevent infinite loops here, but just for a theroretical case. AFAIR we do not allow circular dependencies, BUT we have a test case which creates that case to test, that it doesn't happen. 🤷♂️
| @@ -120,6 +155,27 @@ public function __construct( | |||
| ) { | |||
| } | |||
|
|
|||
| public function canHandle(EventInterface $event): bool | |||
There was a problem hiding this comment.
This is just a workaround, to reduce the subset of possible events, as we are not able to "detect" events, which provide a workspaceName. Also this brings a bit of clearity, which events this catchup hook actually expects.
Neos.Neos/Classes/Fusion/Cache/GraphProjectorCatchUpHookForCacheFlushing.php
Show resolved
Hide resolved
| * | ||
| * @throws NodeTypeNotFound | ||
| */ | ||
| public function registerAssetChange(AssetInterface $asset): void |
There was a problem hiding this comment.
Looking at the tests, asset handling still works, but… why? 😳
There was a problem hiding this comment.
Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php
Show resolved
Hide resolved
| @@ -70,7 +70,7 @@ Feature: ForkContentStream Without Dimensions | |||
|
|
|||
| And the event NodePropertiesWereSet was published with payload: | |||
| | Key | Value | | |||
| | workspaceName | "user" | | |||
| | workspaceName | "user-test" | | |||
There was a problem hiding this comment.
I think it was wrong the whole time, but the test did't fail.
| private function determineParentNodeAggregateIds(ContentRepository $contentRepository, WorkspaceName $workspaceName, NodeAggregateId $childNodeAggregateId, NodeAggregateIds $collectedParentNodeAggregateIds): NodeAggregateIds | ||
| { | ||
| $parentNodeAggregates = $contentRepository->getContentGraph($workspaceName)->findParentNodeAggregates($childNodeAggregateId); | ||
| $parentNodeAggregateIds = NodeAggregateIds::fromArray( | ||
| array_map(static fn (NodeAggregate $parentNodeAggregate) => $parentNodeAggregate->nodeAggregateId, iterator_to_array($parentNodeAggregates)) | ||
| ); | ||
|
|
||
| foreach ($parentNodeAggregateIds as $parentNodeAggregateId) { | ||
| // Prevent infinite loops | ||
| if (!$collectedParentNodeAggregateIds->contain($parentNodeAggregateId)) { | ||
| $collectedParentNodeAggregateIds = $collectedParentNodeAggregateIds->merge(NodeAggregateIds::create($parentNodeAggregateId)); | ||
| $collectedParentNodeAggregateIds = $this->determineParentNodeAggregateIds($contentRepository, $workspaceName, $parentNodeAggregateId, $collectedParentNodeAggregateIds); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| return $collectedParentNodeAggregateIds; | ||
| } | ||
| } |
There was a problem hiding this comment.
These are the ancestorNodeAggregateIds to be correct.
And, if we touch this again, I would suggest to streamline it a bit. Using iteration instead of recursion would make this easier to read and faster, e.g.:
private function determineAncestorNodeAggregateIds(ContentRepository $contentRepository, WorkspaceName $workspaceName, NodeAggregateId $childNodeAggregateId): NodeAggregateIds
{
$contentGraph = $contentRepository->getContentGraph($workspaceName);
/** @var array<NodeAggregate> $stack */
$stack = iterator_to_array($contentGraph->findParentNodeAggregates($childNodeAggregateId));
$ancestorNodeAggregateIds = [];
do {
$nodeAggregate = array_shift($stack);
$ancestorNodeAggregateIds[] = $nodeAggregate->nodeAggregateId;
array_push($stack, ...iterator_to_array($contentGraph->findParentNodeAggregates($nodeAggregate->nodeAggregateId)));
} while ($stack !== []);
return NodeAggregateIds::fromArray($ancestorNodeAggregateIds);
}There was a problem hiding this comment.
btw: Maybe it makes sense to add this method to the ContentGraphInterface...
There was a problem hiding this comment.
We discussed something similar with @kitsunet and @mhsdesign last week. findAncestorNodes on ContentGraphInterface. WDYT?
| public WorkspaceName $workspaceName, | ||
| public NodeAggregateId $nodeAggregateId, | ||
| public NodeTypeName $nodeTypeName, | ||
| public NodeAggregateIds $parentNodeAggregateIds, |
There was a problem hiding this comment.
This should be $ancestorNodeAggregateIds here as well
| WorkspaceName $workspaceName, | ||
| NodeAggregateId $nodeAggregateId, | ||
| NodeTypeName $nodeTypeName, | ||
| NodeAggregateIds $parentNodeAggregateIds |
Cleanup ContentCacheFlusher
ContentCacheFlusheris now only responsible for creatingCacheTagsand flushing them, based on aFlush**Request. No node traversing within theContentCacheFlusheranymore.CacheFlushingStrategyto control the time of "real" flushing, but keeping the interface clean and equal for different cases (immediately or on shutdown).AssetChangeHandlerForCacheFlushingGraphProjectorCatchUpHookForCacheFlushing
ContentCacheFlusher.Workspace*Discardevents.Workspace*Discardevents.CacheTag
CacheTagwhich allows to flush all cache entries of a workspace.NodeCacheEntryIdentifier
AssetChangeHandlerForCacheFlushing
AssetService::assetUpdatedsignal, to handle asset changes.ContentCacheFlusher.Tests
Added tests for:
Resolves #5175 and related issues