-
Notifications
You must be signed in to change notification settings - Fork 29
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
IBX-5388: Fixed performance issues of content updates after field changes #397
Conversation
eZ/Publish/Core/Persistence/Legacy/Tests/Content/MapperTest.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.
The direction here makes perfect sense. We need to test this extensively of course:
Code-wise remarks:
eZ/Publish/Core/Persistence/Legacy/Tests/Content/MapperTest.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Legacy/Tests/Content/MapperTest.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Legacy/Tests/Content/MapperTest.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Legacy/Tests/Content/MapperTest.php
Outdated
Show resolved
Hide resolved
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 1 New issue |
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. All my comments are optional notes that you can ignore as you see fit, since they are mostly opinion-based.
One thing I would very much like to add, but would require effort (probably a lot), is an integration test for it. It would be extremely valuable in my opinion, as we are dealing with a behavior that is very specific and depends on interaction between multiple components.
eZ/Publish/Core/Persistence/Legacy/Tests/Content/MapperTest.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Legacy/Tests/Content/MapperTest.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Legacy/Tests/Content/MapperTest.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Legacy/Tests/Content/MapperTest.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Legacy/Tests/Content/StorageHandlerTest.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.
Following issue found in image asset.
Steps:
- image CT (default), image asset CT (ezstring, ezimageasset)
- create image
- create image asset, select from repository
- edit image CT, add single relation field or keywords
- edit image asset
- open image editor
- flip image
- save as / apply changes to all
Actual result: In UI error occurs "Internal sever error". Exception occurs in log:
php.CRITICAL: Uncaught Error: reset(): Argument #1 ($array) must be of type array, null given {"exception":"[object] (TypeError(code: 0): reset(): Argument #1 ($array) must be of type array, null given at /Users/michalszoltysek/Projects/workspace/ibexa_website_4/vendor/ezsystems/ezplatform-kernel/eZ/Publish/Core/FieldType/Relation/SearchField.php:32)"} []
or
php.CRITICAL: Uncaught Error: implode(): Argument #1 ($pieces) must be of type array, string given {"exception":"[object] (TypeError(code: 0): implode(): Argument #1 ($pieces) must be of type array, string given at /Users/michalszoltysek/Projects/workspace/ibexa_website_4/vendor/ezsystems/ezplatform-kernel/eZ/Publish/Core/FieldType/Keyword/SearchField.php:29)"} []
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.
Retest status.
- ezobjectrelation: OK
- ezkeyword: NOK - the same exception.
For what it's worth, there was a way to process Content update asynchronously, removed as deprecated in the PR: ezsystems/ezpublish-kernel#2976 IMHO, this should be properly handled by asynchronous processing and not by introducing inconsistencies to the Repository API. |
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.
QA Approved on Ibexa Commerce 3.3.38-dev.
d881202
to
0f7966d
Compare
Quality Gate passedIssues Measures |
…nges For more details see https://issues.ibexa.co/browse/IBX-5388 and ezsystems/ezplatform-kernel#397 Key changes: * Fixed performance issues of content updates after field definition changes * Made DefaultDataFieldStorage extend FieldStorage * [Tests] Aligned test coverage with the changes --------- Co-Authored-By: Paweł Niedzielski <[email protected]> Co-Authored-By: Andrew Longosz <[email protected]>
This is an up-merge of ezsystems/ezplatform-kernel#397. See also #376 for more details
v3.3
When new field was added we iterated over each content and applied field changes with default values to each existing version. When we removed the field we iterated over each content and removed data from every version as well. This cause major performance issue as all these changes happened in runtime each time you tried to publish Content Type and grow exponentially with increasing number of content items leading to memory issues, transaction nesting issues and exceeded execution times.
This PR introduces two keystones:
This makes publishing Content Type process much faster and doesn't make content loading process slower (loading times differences are unnoticeable). Change itself is almost transparent to the user and developer and these virtual fields does not need any special handling for day-to-day operations. Once the content is published, copied or anyhow saved, virtual fields become persisted and will be indistinguishable from any other field. But there are some behavior changes, but with low impact:
id
(it remainsnull
),Depends on: https://github.com/ezsystems/ezplatform-version-comparison/pull/85
Checklist:
$ composer fix-cs
).@ezsystems/engineering-team
).