Skip to content
This repository has been archived by the owner on Aug 26, 2022. It is now read-only.

Bug 1177798 - Order contributors by recency of last edit. #3369

Merged
merged 1 commit into from Jul 30, 2015

Conversation

jezdez
Copy link
Contributor

@jezdez jezdez commented Jul 27, 2015

This also moves the contributor bar fetching into a cacheback job.

@jezdez jezdez force-pushed the bug1177798-3 branch 2 times, most recently from 134a619 to ef0bda3 Compare July 27, 2015 12:52
@jezdez
Copy link
Contributor Author

jezdez commented Jul 27, 2015

To be clear, there is another contributor bar in the footer of document pages, which will use the same cacheback job to populate a list of links. That list is not waffled and will use the cacheback job and therefor Memcache and Celery by default. If you think we should waffle the contributor footer bar as well to prevent potential issues with using Memcache/Celery let me know.


<div class="contributor-avatars" data-all-text="{{ _('Show all') }}&hellip;<span class='hidden'>{{ _('contributors') }}</span>" {% if contrib_count > contrib_limit %}data-has-hidden="1"{% endif %}>
<div class="contributor-avatars" data-all-text="{{ _('Show all') }}&hellip;<span class='hidden'>{{ _('contributors') }}</span>" {% if contributors_count > contributor_show_limit %}data-has-hidden="1"{% endif %}>
<span class="quickstat">
{% trans count=contrib_count %}
by {{ count }} contributor:
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to be {{ contributors_count }} now?

@groovecoder
Copy link
Contributor

Spot-check looks good. I also put a debug in the middle of the DocumentContributorsJob.fetch to make sure it was only called once and that subsequent pageviews didn't trigger it.

I am still concerned about the risk that the initial spike of cacheback jobs for every document could trigger memcache overload issues again. It looks like this document.html call is the only place that calls get_contributors, and 0.0002% of visitors to JavaScript clicked a contributor link in the footer.

So, if we put the entire logic of get_contributors, the header, and the footer displays behind the waffle flag, we should be able to throttle this up slowly and watch memcache activity via New Relic, right? (On both production & stage now that stage has memcache)

@jezdez
Copy link
Contributor Author

jezdez commented Jul 29, 2015

@groovecoder So I just made both the header and footer bar depending on the "top_contributors" waffle flag. I moved the check for it to the view since it's easier for the else branch. Can you take a look again?

@jezdez jezdez assigned groovecoder and unassigned jezdez Jul 29, 2015
@jezdez
Copy link
Contributor Author

jezdez commented Jul 29, 2015

@groovecoder BTW we could rate limit the cacheback task to something like 120/m or something

@jezdez jezdez force-pushed the bug1177798-3 branch 4 times, most recently from cdbe9e0 to 0a61c13 Compare July 29, 2015 19:01
@groovecoder
Copy link
Contributor

This looks good to me. I toggled the flag and saw only the first request make the SQL call. If we rate-limit the cacheback job to 120/m, what happens when it hits 121/m? Will it make the next request wait, or just skip refresh from cache?

@jezdez
Copy link
Contributor Author

jezdez commented Jul 30, 2015

The rate limitation happens on Celery's backend level, so what will happen is that Celery will actively prevent from sending the task info to its workers. In other words, the task messages will pile up in RabbitMQ until Celery workers are able to work through them. Since we track the number of RabbitMQ messages with Graphite we'll see if something is wrong.

Cacheback on the other side is smart enough that if a job hasn't produced results it'll set a cache value to an empty value (in our case an empty list) with a TTL of 60 seconds by default. That's the assumed period of time we expect cacheback tasks to be resulting in properly filling the cache with an actual result (even though it may be an empty list just as well). In other words, in those 60 seconds cacheback won't try scheduling another Celery task.

@groovecoder
Copy link
Contributor

Cool. We also have NR RabbitMQ monitoring: https://rpm.newrelic.com/accounts/263620/dashboard/4681572/page/3

This is the first of two steps to getting rid of the profile table and UserProfile model and does a bunch of things:

- moves profile fields into custom user model
- start using django-sundial as an improved timezone field handling for future improvements
- got rid of jsonfield in favor of individual columns for user URLs
- migrates data from profile to user model
- combined profile and user styles
- refactored user model and form code to use static fields instead of dynamically generating them
- simplified userban queries
groovecoder added a commit that referenced this pull request Jul 30, 2015
Bug 1177798 - Order contributors by recency of last edit.
@groovecoder groovecoder merged commit 3684318 into master Jul 30, 2015
@groovecoder groovecoder deleted the bug1177798-3 branch July 30, 2015 20:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants