launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/revision-karma-2 into lp:launchpad.

Commit message:
Ensure allocated_karma() is called for all revisions

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1050191 in Launchpad itself: "allocate-revision-karma.py is too slow to complete"
  https://bugs.launchpad.net/launchpad/+bug/1050191

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/revision-karma-2/+merge/137906

allocate-revision-karma.py is having its DB connection reaped while it's
running, because the query in RevisionSet
getRevisionsNeedingKarmaAllocated is terribly slow.

Right now, we have 21.1 million revisions. 15.9 million of these are
flagged as not having karma allocated. These 15.9 million revisions are
the combination of revisions that have never been processed, or only
exist in +junk branches, or only exist in branches owned by invalid
people. Each run, the scanner needs to check all 15.9 million revisions.

--------------------------------------------------------------------

RULES

    Pre-implementation: stub
    * This is a follow up branch to address a case seen on qastaging.
      * The script assumes that returning None from allocateKarma()
        is bad because Lp will eternally process the revision. This
        is not bad anymore because the method has set rev.allocated_karma
        to True.
      * The script will look up the revision's branch, but since lookup
        ignores junk branches, the branch can be None. This is okay
        because we want to ensure rev.allocated_karma is still called
        to ensure Lp does not reprocess the revision.

QA

    * As a webops to run
      ./cronscripts/allocate-revision-karma.py
      for qastaging's script host (gandwana).
    * Verify there are no errors after a 5 minutes. The proc
      can be killed since we do not need to Verify all 15 million
      revisions.

LoC

    I have a 10,000 line credit this week.


LINT

    lib/lp/code/model/revision.py
    lib/lp/code/model/tests/test_revision.py
    lib/lp/code/scripts/revisionkarma.py


TEST

    ./bin/test -vc -t TestRevisionKarma lp.code.model.tests.test_revision
    ./bin/test -vc lp.code.scripts.tests.test_revisionkarma


IMPLEMENTATION

I updated allocateKarma() to not attempt to add karma if there is no
community branch for the revision. I updated the script to continue
processing revisions because the call to allocateKarma() did set
rev.allocated_karma to True.
    lib/lp/code/model/revision.py
    lib/lp/code/model/tests/test_revision.py
    lib/lp/code/scripts/revisionkarma.py
-- 
https://code.launchpad.net/~sinzui/launchpad/revision-karma-2/+merge/137906
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/revision-karma-2 into lp:launchpad.

=== modified file 'lib/lp/code/model/revision.py'
--- lib/lp/code/model/revision.py	2012-12-03 18:33:11 +0000
+++ lib/lp/code/model/revision.py	2012-12-04 16:31:31 +0000
@@ -133,7 +133,7 @@
         self.karma_allocated = True
         # If we know who the revision author is, give them karma.
         author = self.revision_author.person
-        if author is not None:
+        if author is not None and branch is not None:
             # Backdate the karma to the time the revision was created.  If the
             # revision_date on the revision is in future (for whatever weird
             # reason) we will use the date_created from the revision (which

=== modified file 'lib/lp/code/model/tests/test_revision.py'
--- lib/lp/code/model/tests/test_revision.py	2012-12-03 19:37:01 +0000
+++ lib/lp/code/model/tests/test_revision.py	2012-12-04 16:31:31 +0000
@@ -171,6 +171,15 @@
         karma = rev.allocateKarma(branch)
         self.assertIs(None, karma)
 
+    def test_allocateKarma_personal_branch_none(self):
+        # Revisions only associated with junk branches don't get karma,
+        # and the branch may be None because the revision_set does not
+        # attempt to get junk branches.
+        author = self.factory.makePerson()
+        rev = self.factory.makeRevision(author=author)
+        karma = rev.allocateKarma(None)
+        self.assertIs(None, karma)
+
     def test_allocateKarma_package_branch(self):
         # A revision on a package branch gets karma.
         author = self.factory.makePerson()

=== modified file 'lib/lp/code/scripts/revisionkarma.py'
--- lib/lp/code/scripts/revisionkarma.py	2010-10-12 05:34:49 +0000
+++ lib/lp/code/scripts/revisionkarma.py	2012-12-04 16:31:31 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The actual script class to allocate revisions."""
@@ -23,15 +23,6 @@
         There are a number of circumstances where this doesn't happen:
           * The revision author is not linked to a Launchpad person
           * The branch is +junk
-
-        When a branch is moved from +junk to a project we want to be able to
-        allocate karma for the revisions that are now in the project.
-
-        When a person validates an email address, a link is made with a
-        `RevisionAuthor` if the revision author has that email address.  In
-        this situation we want to allocate karma for the revisions that have
-        the newly linked revision author as the and allocate karma for the
-        person.
         """
         self.logger.info("Updating revision karma")
 
@@ -49,14 +40,7 @@
                 # allocate karma for junk branches.
                 branch = revision.getBranch(
                     allow_private=True, allow_junk=False)
-                karma = revision.allocateKarma(branch)
-                if karma is None:
-                    error_msg = (
-                        'No karma generated for revid: %s (%s)' %
-                        (revision.revision_id, revision.id))
-                    self.logger.critical(error_msg)
-                    # Stop now to avoid infinite loop.
-                    raise AssertionError(error_msg)
+                revision.allocateKarma(branch)
                 count += 1
             self.logger.debug("%s processed", count)
             transaction.commit()