IBX-704: Added aggregation support to NodeFactory #1792

mateuszbieniek · 2021-07-02T09:50:14Z

Jira's issue: https://issues.ibexa.co/browse/IBX-704

requires:
ezsystems/ezplatform-kernel#215
ezsystems/ezplatform-solr-search-engine#215

Children counts call to Solr can create an overhead for load-subtree calls.
This PR adds Aggregation support for NodeFactory to speed it up when supported.

lserwatka · 2021-07-02T09:58:29Z

@webhdx would be great for reviewing this next week.

mateuszbieniek · 2021-07-19T09:40:25Z

ping @ezsystems/engineering-team

webhdx

I'm not too experienced with SOLR/Elastic usage so it needs attention from Adam or Andrew. Anyway I don't quite like the general idea here. NodeFactory becomes more and more complicated/bloated with all performance improvements. Would be nice to provide alternative NodeFactory which could utilize different handling when ES/SOLR is being used. But I understand this would require more refactoring and providing some extension points. I put some remarks in the comments, please consider them.

webhdx · 2021-07-26T10:19:03Z

src/lib/UI/Module/ContentTree/NodeFactory.php

@@ -33,6 +34,7 @@ final class NodeFactory
        'DatePublished' => SortClause\DatePublished::class,
        'ContentName' => SortClause\ContentName::class,
    ];
+    private const MAX_AGGREGATED_LOCATION_IDS = 100;


I think this should be configurable if there is possibility to fine tune performance on different setups.

src/lib/UI/Module/ContentTree/NodeFactory.php

webhdx · 2021-07-26T10:27:40Z

src/lib/UI/Module/ContentTree/NodeFactory.php

+     *
+     * @throws \eZ\Publish\API\Repository\Exceptions\InvalidArgumentException
+     */
+    private function supplyChildrenCount(Node $node, array $aggregationResult = null): void


It would be cleaner if you used empty array instead of null value.

Passing null here indicates that Aggregation results are not available (for example: using a legacy search engine) and the "legacy" way of counting children should be used inside the supplyChildrenCount method. An empty array is a totally valid result of aggregation. I can change it to pass to an empty array, but we will have to introduce something like $isAggregationSupported and pass it alongside to supplyChildrenCount

It would be cleaner if you used empty array instead of null value.

Passing null here indicates that Aggregation results are not available (for example: using a legacy search engine) and the "legacy" way of counting children should be used inside the supplyChildrenCount method. An empty array is a totally valid result of aggregation. I can change it to pass to an empty array, but we will have to introduce something like $isAggregationSupported and pass it alongside to supplyChildrenCount

@mateuszbieniek I don't see how distinguishing between [] and null would change the behavior of this method in its current state. Could you show a snippet with [] instead of null?

There are two possibilities:

Aggregations are not supported ($aggregationResult is null). Every single container node will have $this->countSubitems($node->locationId); called

Aggregations are supported and $aggregationResult is an array.

$totalCount = isset($aggregationResult[$node->locationId]) ? $aggregationResult[$node->locationId] : 0;

If $aggregationResult has key $node->locationId then node has children, if a key does not exist, this means that there children count is 0;

So, if I pass it as empty array, when aggregations are not supported current state of method will result on 0 children count for all nodes.

I can change it to an empty array but will have to add a check for aggregation support inside supplyChildrenCount method.

I've changed the check inside from supplyChildrenCount:

if ($aggregationResult) {

to

if ($aggregationResult !== null) {

This should reduce the confusion.

src/lib/UI/Module/ContentTree/NodeFactory.php

alongosz · 2021-07-28T14:54:18Z

src/lib/UI/Module/ContentTree/NodeFactory.php

+     *
+     * @throws \eZ\Publish\API\Repository\Exceptions\InvalidArgumentException
+     */
+    private function supplyChildrenCount(Node $node, array $aggregationResult = null): void


It would be cleaner if you used empty array instead of null value.

Passing null here indicates that Aggregation results are not available (for example: using a legacy search engine) and the "legacy" way of counting children should be used inside the supplyChildrenCount method. An empty array is a totally valid result of aggregation. I can change it to pass to an empty array, but we will have to introduce something like $isAggregationSupported and pass it alongside to supplyChildrenCount

@mateuszbieniek I don't see how distinguishing between [] and null would change the behavior of this method in its current state. Could you show a snippet with [] instead of null?

adamwojs

Could you please share some basic numbers in PR description as this is an optimisation patch?

src/lib/UI/Module/ContentTree/NodeFactory.php

mateuszbieniek · 2021-08-04T12:17:43Z

SonarCloud Quality Gate failed.

1 Bug
0 Vulnerabilities
2 Security Hotspots
83 Code Smells

No Coverage information
2.4% Duplication

Those are false positives - SonarCloud messes something up...

src/lib/UI/Module/ContentTree/NodeFactory.php

Co-authored-by: Adam Wójs <adam@wojs.pl>

alongosz

With the change to the method behavior it's clear why you need it there. With test coverage we could avoid this discussion, because you would spot a mistake immediately.
One final request related to forward compatibility of rebranding:

src/bundle/Resources/config/default_parameters.yaml

…y.max_location_ids_in_single_aggregation

sonarcloud · 2021-08-05T09:09:46Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

lserwatka · 2021-08-09T12:10:53Z

You can merge it up now.

* Added aggregation support to NodeFactory * Changed to use LocationChildrenTermAggregation * LocationChildrenTermAggregation moved to different namespace in kernel * Changes after CR * CS Fix * Changes after CR#3 * CS fix * Fixups after CR * Update src/lib/UI/Module/ContentTree/NodeFactory.php Co-authored-by: Adam Wójs <adam@wojs.pl> * Changed the parameter name to ibexa.admin_ui.content_tree.node_factory.max_location_ids_in_single_aggregation Co-authored-by: Adam Wójs <adam@wojs.pl> (cherry picked from commit 64b5a09)

Added aggregation support to NodeFactory

0c48fff

mateuszbieniek marked this pull request as draft July 2, 2021 09:50

mateuszbieniek mentioned this pull request Jul 19, 2021

IBX-704: Added LocationChildrenTermAggregation ezsystems/ezplatform-kernel#215

Merged

Changed to use LocationChildrenTermAggregation

eaaedde

mateuszbieniek changed the title ~~[WIP] Added aggregation support to NodeFactory~~ IBX-704: Added aggregation support to NodeFactory Jul 19, 2021

mateuszbieniek marked this pull request as ready for review July 19, 2021 09:23

mateuszbieniek mentioned this pull request Jul 19, 2021

IBX-704: Added support for LocationChildrenAggregation ezsystems/ezplatform-solr-search-engine#215

Merged

mateuszbieniek added Improvement Ready for review labels Jul 19, 2021

LocationChildrenTermAggregation moved to different namespace in kernel

9082b5c

alongosz requested review from adamwojs, webhdx, mikadamczyk, Nattfarinn, ViniTou and alongosz July 26, 2021 09:54

webhdx requested changes Jul 26, 2021

View reviewed changes

mateuszbieniek added 2 commits July 26, 2021 13:06

Changes after CR

50e062e

CS Fix

2f7164f

alongosz requested changes Jul 28, 2021

View reviewed changes

adamwojs reviewed Jul 29, 2021

View reviewed changes

src/lib/UI/Module/ContentTree/NodeFactory.php Outdated Show resolved Hide resolved

src/lib/UI/Module/ContentTree/NodeFactory.php Outdated Show resolved Hide resolved

src/lib/UI/Module/ContentTree/NodeFactory.php Outdated Show resolved Hide resolved

mateuszbieniek and others added 3 commits July 29, 2021 14:49

Changes after CR#3

b99d24d

CS fix

6e089e2

Fixups after CR

cd9d733

mateuszbieniek requested review from alongosz, webhdx and adamwojs August 4, 2021 12:16

adamwojs approved these changes Aug 5, 2021

View reviewed changes

src/lib/UI/Module/ContentTree/NodeFactory.php Show resolved Hide resolved

Update src/lib/UI/Module/ContentTree/NodeFactory.php

569292a

Co-authored-by: Adam Wójs <adam@wojs.pl>

mateuszbieniek changed the base branch from master to 2.2 August 5, 2021 08:45

alongosz requested changes Aug 5, 2021

View reviewed changes

src/bundle/Resources/config/default_parameters.yaml Outdated Show resolved Hide resolved

mateuszbieniek changed the base branch from 2.2 to 2.3 August 5, 2021 09:03

Changed the parameter name to ibexa.admin_ui.content_tree.node_factor…

983479c

…y.max_location_ids_in_single_aggregation

alongosz approved these changes Aug 5, 2021

View reviewed changes

mateuszbieniek added the Ready for QA label Aug 5, 2021

bogusez self-assigned this Aug 5, 2021

Nattfarinn approved these changes Aug 6, 2021

View reviewed changes

bogusez approved these changes Aug 6, 2021

View reviewed changes

bogusez added QA approved and removed Ready for QA labels Aug 6, 2021

lserwatka merged commit 64b5a09 into ezsystems:2.3 Aug 9, 2021

mateuszbieniek deleted the load_subtree_aggregation_support branch August 9, 2021 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBX-704: Added aggregation support to NodeFactory #1792

IBX-704: Added aggregation support to NodeFactory #1792

mateuszbieniek commented Jul 2, 2021 •

edited

lserwatka commented Jul 2, 2021

mateuszbieniek commented Jul 19, 2021

webhdx left a comment

webhdx Jul 26, 2021

webhdx Jul 26, 2021

mateuszbieniek Jul 26, 2021

alongosz Jul 28, 2021

mateuszbieniek Jul 29, 2021

mateuszbieniek Jul 29, 2021

mateuszbieniek Aug 4, 2021

alongosz Jul 28, 2021

adamwojs left a comment

mateuszbieniek commented Aug 4, 2021

alongosz left a comment

sonarcloud bot commented Aug 5, 2021

lserwatka commented Aug 9, 2021

IBX-704: Added aggregation support to NodeFactory #1792

IBX-704: Added aggregation support to NodeFactory #1792

Conversation

mateuszbieniek commented Jul 2, 2021 • edited

lserwatka commented Jul 2, 2021

mateuszbieniek commented Jul 19, 2021

webhdx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamwojs left a comment

Choose a reason for hiding this comment

mateuszbieniek commented Aug 4, 2021

alongosz left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Aug 5, 2021

lserwatka commented Aug 9, 2021

mateuszbieniek commented Jul 2, 2021 •

edited