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

Switch get_node_details to always use cache when available... #1268

Merged
merged 8 commits into from
Mar 19, 2019

Conversation

kollivier
Copy link
Contributor

…and update cache values async.

Description

Since get_node_details is often called on the root node and can take significant time, switch to always preferring cache results when available.

Issue Addressed (if applicable)

A whole bunch of Sentry reports about get_topic_details timing out ;-)

Steps to Test

  • Add a node or nodes to a channel.
  • Check details in channels page, they should be the old values
  • Check again a short time later - they should be updated.

Implementation Notes (optional)

At a high level, how did you implement this?

Much of this PR is shuffling code around, as in order to call get_node_details as a task I needed to move it out of views/nodes.py in order to avoid a circular import dependency between views/nodes.py and tasks.py. For similar reasons, I needed to move the thumbnail handling code outside of views/nodes.py as well.

The best place it seemed to put these methods was within the models themselves, so I added ContentNode.get_details and ContentNode.get_thumbnail. I also switched the get_details function to call Channel.get_thumbnail() instead of using the get_channel_thumbnail function in views/nodes.py, which could then be removed.

Lastly, I added tests ensuring that get_details() still returns the relevant information, that calling it from the get_topic_details endpoint updates the cache when needed, and that the get_thumbnail model methods return the correct thumbnail info.

Checklist

Delete any items that don't apply

  • Is the code clean and well-commented?
  • Have the changes been added to the CHANGELOG?
  • Are there tests for this change?

Reviewers

If you are looking to assign a reviewer, here are some options:

  • Jordan jayoshih (full stack)
  • Aron aronasorman (back end, devops)
  • Micah micahscopes (full stack)
  • Kevin kollivier (back end)
  • Ivan ivanistheone (Ricecooker)
  • Richard rtibbles (full stack, Kolibri)
  • Radina @radinamatic (documentation)

@kollivier kollivier requested a review from jayoshih March 7, 2019 00:35
Copy link
Contributor

@jayoshih jayoshih left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for getting this out so quickly! Some super minor stuff, but otherwise should be good to merge

def get_thumbnail(self):
# Problems with json.loads, so use ast.literal_eval to get dict
if self.thumbnail_encoding:
thumbnail_data = ast.literal_eval(self.thumbnail_encoding)
Copy link
Contributor

Choose a reason for hiding this comment

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

I did implement a function to handle this without relying on ast.literal_eval. Do you mind switching to this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do! :)

Copy link
Member

Choose a reason for hiding this comment

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

Note that for channel thumbnails I migrated the thumbnail_encoding field to a JSON field to avoid this malarkey: #1140

The data migrations that I made to facilitate this should transparently transfer to the content node models also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was thinking about doing this, but then I thought that if we go the route of migrating to all files for ContentNode thumbnails, we can just create a migration to remove thumbnail_encoding entirely on ContentNode. :)

contentcuration/contentcuration/models.py Outdated Show resolved Hide resolved
@kollivier kollivier requested a review from jayoshih March 7, 2019 01:11
@rtibbles
Copy link
Member

rtibbles commented Mar 7, 2019

Not in the scope of this PR, but I note that we are currently using the database cache backend: https://github.com/learningequality/studio/blob/develop/contentcuration/contentcuration/settings.py#L94

I imagine this may give us a slight performance boost because it will always be one query, than potentially many - but this will still be much slower than using an in memory backend (I believe @kollivier had previously suggested https://github.com/sebleier/django-redis-cache for such purposes - he's a smart guy, so I'd definitely listen to him!)

@kollivier kollivier merged commit 8bfed21 into learningequality:develop Mar 19, 2019
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.

3 participants