Skip to content
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-29851: As a developer I want to be able to load several Locations at once, fast #2493

Merged
merged 3 commits into from Dec 20, 2018

Conversation

andrerom
Copy link
Contributor

@andrerom andrerom commented Dec 3, 2018

Question Answer
JIRA issue EZP-29851
New feature yes
Target version master
BC breaks no
Tests pass yes
Doc needed maybe

Motivation

Like the other bulk loading API's added recently this is to optimize usage in:

  • Page builder
  • Cron Jobs / Commands
  • GraphQL
  • REST
  • Admin UI
  • Internally in Repository itself
  • ...

Any place where several content / locations might be loaded today.

These changes allows for less PHP code to execute, less SQL lookups, and less cache lookups by taking advantage of Symfony Cache's multi get support.

Biggest benefit will be on setups where cache server is on a different machine, this can save ~5ms
for each and every lookup saved. Which can on complex landing pages mean saving up-to several seconds of load time when HTTPCache is cold or disabled.

There are a few other follow ups on this coming later in other parts of the system, for instance getting rid of Symfon's tag lookup.

Design

Like other multi load API's return iterable list of objects, in this case Locations.
Iterable as we might want to introduce custom collection in the future to expose more info, hence avoid hardcoding usage of plain PHP array type.

     /**
      * Return list of unique Locations, with location id as key.
      *
      * Missing items (NotFound) & those user does not have access to (Unauthorized), will be missing from the
      * list and not cause any exception. It's up to calling logic to determine if this should cause exception or not.
      *
      * @param array $locationIds
      * @param string[]|null $prioritizedLanguages Filter on and use as prioritized language code on translated properties of returned objects.
      * @param bool|null $useAlwaysAvailable Respect always available flag on content when filtering on $prioritizedLanguages.
      *
      * @return \eZ\Publish\API\Repository\Values\Content\Location[]|iterable
      */
     public function loadLocationList(array $locationIds, array $prioritizedLanguages = null, bool $useAlwaysAvailable = null): iterable;

TODO:

@andrerom andrerom changed the base branch from lazy-loading-not-avail-content to master December 4, 2018 12:45
*
* @return \eZ\Publish\API\Repository\Values\Content\Location[]|iterable
*/
public function loadLocationList(array $locationIds, array $prioritizedLanguages = null, bool $useAlwaysAvailable = null): iterable;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The loadLocationList method is missing on \eZ\Publish\Core\REST\Client\LocationService

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 6cb281cc3dc08714a0b309cd20865165e2ca3122

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, one request:

… at once, fast

Motivation
----------
Like the other bulk loading API's added recently this is to optimize usage in:
- Page builder
- Cronjobs / Commands
- GraphQL
- REST
- Admin UI
- Internally in Repository itself
- ...

These changes allows for less PHP code to execute, less SQL lookups, and less cache lookups by taking advantage of
Symfony Cache's multi get support.

Biggest benfit will be on setups where cache server is on a different machine, this can save ~5ms
for each and every lookup saved. Which can on complex landing pages mean saving up-to several seconds
of load time when HTTPCache is cold or disabled.

Design
------
Like other multi load API's return iterable list of objects, in this case Locations.
_Iterable as we might want to introduce custom collection in the future to expose more info, hence avoid hardcoding usage of plain PHP array type._

```
     /**
      * Return list of unique Locations, with location id as key.
      *
      * Missing items (NotFound) & those user does not have access to (Unauthorized), will be missing from the
      * list and not cause any exception. It's up to calling logic to determine if this should cause exception or not.
      *
      * @param array $locationIds
      * @param string[]|null $prioritizedLanguages Filter on and use as prioritized language code on translated properties of returned objects.
      * @param bool|null $useAlwaysAvailable Respect always available flag on content when filtering on $prioritizedLanguages.
      *
      * @return \eZ\Publish\API\Repository\Values\Content\Location[]|iterable
      */
     public function loadLocationList(array $locationIds, array $prioritizedLanguages = null, bool $useAlwaysAvailable = null): iterable;
```
@micszo micszo assigned micszo and m-tyrala and unassigned micszo Dec 19, 2018
Copy link

@m-tyrala m-tyrala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sanities passed

@lserwatka lserwatka merged commit 3e0b856 into master Dec 20, 2018
@lserwatka lserwatka deleted the location_multi_load branch December 20, 2018 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants