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

Mimi #140

Merged
merged 7 commits into from
Jul 18, 2017
Merged

Mimi #140

merged 7 commits into from
Jul 18, 2017

Conversation

sherakama
Copy link
Member

@sherakama sherakama commented Jun 6, 2017

READY FOR REVIEW

Summary

  • // Some sites have wwaaayyy too many items in the capx_profiles table for this
  • // to load with an appropriate amount of memory. You can thank Zach Chandler
  • // for finding out this edge case. As such, we will apply two different
  • // stategies for fetching the CAPx metadata. If the amount of profile entries
  • // are over a value then switch from one big cached call to multiple pings of
  • // the database. We are trading PHP memory for hammering the DB with multiple
  • // queries.

Needed By (Date)

  • July 1st.

Urgency

  • Moderate.

Steps to Test

  1. Clone https://sites.stanford.edu/mimisbrunnr/ to local
  2. Check out this branch
  3. Play with threshold setting

Affected Projects or Products

  • Not really. Just Mimi.

Associated Issues and/or People

  • None.

See Also

@sherakama
Copy link
Member Author

@boznik,

I've added you to review this approach. So this issue was caused by someone importing ALL publications from EVERYONE on the CAP API. Yep, that was Zach. Anyhow, this caused the capx_profiles table to be very large (~240000). That amount of data is too large to pull all at once but a single request is more efficient when the amount of data is lower. I have introduced a threshold variable to swap out strategies. Please have a review.

@jbickar
Copy link
Contributor

jbickar commented Jun 16, 2017

Undefined offset: 4 in stanford_capx_entity_load() (line 292 of stanford_capx/stanford_capx.entity.hooks.inc

@jbickar
Copy link
Contributor

jbickar commented Jun 16, 2017

I'm trying to reproduce the Undefined offset, but I also get this with drush rr (on mimi):

% drush rr
The registry has been rebuilt via registry_rebuild (A).                                                                                                                                                                    [success]
array_flip(): Can only flip STRING and INTEGER values! entity.controller.inc:742                                                                                                                                           [warning]
array_flip(): Can only flip STRING and INTEGER values! entity.controller.inc:219                                                                                                                                           [warning]
array_flip(): Can only flip STRING and INTEGER values! entity.controller.inc:742                                                                                                                                           [warning]
array_flip(): Can only flip STRING and INTEGER values! entity.controller.inc:219                                                                                                                                           [warning]
All caches have been cleared with drush_registry_rebuild_cc_all.                                                                                                                                                           [success]
The registry has been rebuilt via drush_registry_rebuild_cc_all (B).                                                                                                                                                       [success]
array_flip(): Can only flip STRING and INTEGER values! entity.controller.inc:742                                                                                                                                           [warning]
array_flip(): Can only flip STRING and INTEGER values! entity.controller.inc:219                                                                                                                                           [warning]
array_flip(): Can only flip STRING and INTEGER values! entity.controller.inc:742                                                                                                                                           [warning]
array_flip(): Can only flip STRING and INTEGER values! entity.controller.inc:219                                                                                                                                           [warning]
All caches have been cleared with drush_registry_rebuild_cc_all.                                                                                                                                                           [success]
All registry rebuilds have been completed.                                                                                                                                                                                 [success]

@jbickar
Copy link
Contributor

jbickar commented Jun 16, 2017

I can reproduce the Undefined offset notice on the mimi homepage.

Clearing the cache from the GUI gives:
Notice: Undefined property: CAPx\Drupal\Entities\CFEntityType::$machine_name in capx_filters_entity_presave() (line 343 of /Users/jbickar/Sites/mimisbrunnr/sites/default/modules/stanford/stanford_capx/modules/capx_filters/capx_filters.module).

@zchandler
Copy link
Contributor

Thanks Guys!

@sherakama
Copy link
Member Author

@jbickar
You should no longer get the PHP errors. Thanks for finding those.

@sherakama sherakama requested review from jbickar and removed request for boznik June 28, 2017 22:55
Copy link
Contributor

@jbickar jbickar left a comment

Choose a reason for hiding this comment

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

Works great; nice work.

@@ -330,7 +330,7 @@ function capx_filters_parse_date($string) {
function capx_filters_entity_presave($entity, $type) {

// We only want to act on importer cfe entities.
if ($type !== 'capx_cfe' && $entity->type !== 'importer') {
if ($type !== 'capx_cfe' || $entity->type !== 'importer') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a fix to bad logic, unrelated to the memory issue, yeah?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, Was acting on too many types.

->execute()
->fetchField();

if ($count > variable_get('stanford_capx_entity_load_limit', 10000)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Fun times:
stanford_capx_entity_load_limit = 1000, everything is wunderbar
stanford_capx_entity_load_limit = 100000, things start to get wonky
stanford_capx_entity_load_limit = 1000000, WSOD

That's pretty neat.

@jbickar jbickar merged commit 77bd6e8 into php54-2.x Jul 18, 2017
@jbickar jbickar deleted the mimi branch July 18, 2017 19:55
@sherakama
Copy link
Member Author

Thanks for reviewing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants