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

Bug 1183082 - Fix attachment feeds.#3341

Merged
jezdez merged 2 commits intomasterfrom
bug1183082
Jul 13, 2015
Merged

Bug 1183082 - Fix attachment feeds.#3341
jezdez merged 2 commits intomasterfrom
bug1183082

Conversation

@jezdez
Copy link
Copy Markdown
Contributor

@jezdez jezdez commented Jul 13, 2015

No description provided.

@darkwing
Copy link
Copy Markdown
Contributor

@jezdez "attachment" ! :)

@jezdez jezdez changed the title Bug 1183082 - Fix attachement feeds. Bug 1183082 - Fix attachment feeds. Jul 13, 2015
Comment thread kuma/attachments/feeds.py
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would select_related be more efficient here, since creator and attachment are both ForeignKey relations? select_related would get them in the same query with a SQL join, rather than prefetch_related doing separate lookups for each and joining in Python?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, select_related would do this in one query, but since we're running on a MySQL cluster with query cache on it would essentially invalidate the query cache if any of the inner join tables is changed, e.g. if a new user is added or even just updated.

So assuming a case in which a new user signs up, using prefetch_related here would only trigger the invalidation of the query cache for the creator prefetch query. While the other two for the attachment revision and the attachment would come straight from memory. All in all this is a micro optimization that given the number of queries we do against MySQL may not matter, but it's nicer to use the query cache as much as possible and live with the slight overhead on the Python side.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense. r+

@groovecoder
Copy link
Copy Markdown
Contributor

r+ with comments

@groovecoder groovecoder assigned jezdez and unassigned groovecoder Jul 13, 2015
jezdez added a commit that referenced this pull request Jul 13, 2015
Bug 1183082 - Fix attachment feeds.
@jezdez jezdez merged commit f5098a2 into master Jul 13, 2015
@jezdez jezdez deleted the bug1183082 branch July 13, 2015 18:48
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.

3 participants