Review Board

beta

Add support for default reviewers

Updated 1 year, 2 months ago

David Trowbridge Reviewers
trunk reviewboard
30
None Review Board SVN 
This is a generic solution to the question of adding default reviewers.
It's generic enough to be useful both for installations like reviews.r-b.o
where there's a single group for all reviews, and the extraordinarily
complex case we've got at VMware, where different groups own different
parts of a single huge codebase.

There's a new model which basically has a regular expression and a list
of default people/groups.  When a new diff is created, it iterates through
the list of filenames in the diff and checks them against each of the
DefaultReviewer regexps.  If one matches, those defaults are added to the
draft's list of target reviewers.
Did a couple tests with different diffs to make sure it added the correct
reviewers.  This might blow up with diffs that contain added or removed
files, I'm not sure.  I'll do some more testing with that before I commit.
Christian Hammond
Posted 1 year, 2 months ago (October 15th, 2007, 2:26 a.m.)
Sweet. With this, we'll be able to add a --publish option to post-review that just lets you fire and forget a review request in most cases.
Loading diff fragment... 
Even if they change, we could do something about that. Keep a dict of regex -> compiled regex mappings and just check if it's in there, then use the compiled one. Though we'd want to purge this on occasion.
  1. David Trowbridge 1 year, 2 months ago (October 15th, 2007, 9:47 a.m.) 
    I think for now, I'm going to not try to do this.  If this becomes a bottleneck
we can optimize it, but a TODO is good enough at the moment.
Loading diff fragment... 
I believe calling self.diffset.files.all() each time will cause a database hit. We can optimize this by storing the queryset outside the first for loop.

filediffs = self.diffset.files.all()

for default in DefaultReviewer.objecs.all():
    for filediff in filediffs:


filediffs will contain the cached entries, whereas self.diffset.files.all() should hit the database each time the queryset is recreated.

We may want to also compile the regex at the start of the first for loop.
  1. David Trowbridge 1 year, 2 months ago (October 15th, 2007, 9:47 a.m.) 
    Done.
Loading diff fragment... 
This is inefficient for the same reason as above.
  1. David Trowbridge 1 year, 2 months ago (October 15th, 2007, 9:48 a.m.) 
    I think it actually caches the results, but it doesn't hurt.  Done.
  2. Christian Hammond 1 year, 2 months ago (October 18th, 2007, 11:31 a.m.) 
    I thought it did from what I read too, but when investigating the memory leak, the only caching I could find was on the MySQL backend side, so I'm not sure this is always the case. The queryset, however, is guaranteed to cache them for the lifetime of the queryset.

Anyhow, this looks good. Ship it :)