Optimize how review request and review participants are calculated.

Review Request #10359 — Created Dec. 13, 2018 and submitted

chipx86
Review Board
release-3.0.x
3beaf85...
reviewboard

Both ReviewRequest and Review have a participants attribute that
returned the list of users who participated in the discussion. For
review requests, this included anyone who has reviewed the review
request or replied to a review, and for reviews this includes both the
reviewer and anyone who has replied.

These attributes were old and slow. Review.participants performed one
SQL query to get the list of replies (looking up all attributes in the
process) and one SQL query per reply for the user. Worse,
ReviewRequest.participants performed that same number of lookups per
review, making it particularly slow for large review requests. Neither
filtered out any duplicates, so if the same user replied 10 times, their
user information would be fetched and returned 10 times.

They also suffered from a bug where people with pending drafts of
reviews or replies would be included in the results, prematurely
including them.

This change optimizes these methods, but retains their compatibility
with a deprecation warning. It also introduces replacements that can be
used going forward.

The old methods/attributes have been updated to more carefully craft
single queries that provide the same results without pulling in and
re-querying so much data. They still suffer from factoring in
unpublished reviews/replies, in order to maintain compatibility, but
ultimately we want to get rid of these. To help with that, we now
provide deprecation warnings stating that these will be removed in 4.0.

The new ReviewRequest.review_participants and
Review.all_participants attributes are cached properties that return
sets, rather than lists, of participants. They won't contain duplicate
entries, and no order is ever guaranteed, leaving such things up to the
caller.

They each have upwards of two SQL queries: One for for a list of
user IDs associated with the reviews/replies, and one for the actual
User entries corresponding to those IDs. We do this in order to avoid
a JOIN, speeding up the queries a bit so we can better leverage indexes
and SQL query caches. If a review request lacks reviews, or a review
lacks replies from users other than the original reviewer, then the
second SQL statement won't even be performed.

Call sites within Review Board have been updated to use the new
attributes. Both the new and old ones now have full test coverage.

All unit tests pass.

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

chipx86
david
  1. Ship It!
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (d0ddb24)
Loading...