Merge lp:~gmb/launchpad/bugwatch-next_check-bug-544238 into lp:launchpad/db-devel

[Graham Binns]

Add a next_check column to BugWatch.

This branch adds a next_check field to BugWatch. We'll use this to allow us to throttle BugWatch updates where the BugWatch is continually failing.

[Graham Binns (gmb)]

Argh. I've forgotten to add the prerequisite branch to this one. Here's the actual diff for this branch:

=== added file 'database/schema/patch-2207-99-0.sql'
--- database/schema/patch-2207-99-0.sql 1970-01-01 00:00:00 +0000
+++ database/schema/patch-2207-99-0.sql 2010-03-22 15:58:29 +0000
@@ -0,0 +1,7 @@
+SET client_min_messages=ERROR;
+
+ALTER TABLE BugWatch ADD COLUMN next_check timestamp without time zone DEFAULT timezone('UTC'::text, now());
+
+CREATE INDEX bugwatch__next_check__idx ON BugWatch(next_check);
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 99, 0);

[Jonathan Lange (jml)]

Why aren't you using the Job table for this?

Even if you don't, consider calling the column 'scheduled_start' to at least be vaguely consistent.

[Graham Binns (gmb)]

On 22 March 2010 16:10, Jonathan Lange <email address hidden> wrote:
> Why aren't you using the Job table for this?
>

Is there a reason to use the Job table? We decided last week that
checkwatches shouldn't use the Jobs system; it doesn't fit the needs
of our batched processing model particularly well and any such
solution would be a kludge.

I'll happily rename the field, though.

[Jonathan Lange (jml)]

On Mon, Mar 22, 2010 at 4:15 PM, Graham Binns <email address hidden> wrote:
> On 22 March 2010 16:10, Jonathan Lange <email address hidden> wrote:
>> Why aren't you using the Job table for this?
>>
>
> Is there a reason to use the Job table? We decided last week that
> checkwatches shouldn't use the Jobs system; it doesn't fit the needs
> of our batched processing model particularly well and any such
> solution would be a kludge.
>

Oh right, I saw that decision mentioned on a bug report last week but
didn't see anything about the reasoning behind it. I'm surprised that
a system which bears such a strong resemblance to code imports
requires a significantly different model. Perhaps the resemblance is
only superficial.

> I'll happily rename the field, though.

Thanks. You might want to hold off until stub & Bjorn have their say though.

jml

[Graham Binns (gmb)]

On 22 March 2010 16:21, Jonathan Lange <email address hidden> wrote:
> Oh right, I saw that decision mentioned on a bug report last week but
> didn't see anything about the reasoning behind it. I'm surprised that
> a system which bears such a strong resemblance to code imports
> requires a significantly different model. Perhaps the resemblance is
> only superficial.

We thought the same thing to, but BugWatch is significantly different
in that it loads a lot of data from a bug tracker up front and then
does bug watch updates based on the cached data. Using the Jobs model
would mean that we wouldn't (easily) have that cache, which in turn
means that we'd be hitting bug trackers for data a lot more than we do
now. That's okay for Bugzilla 3.4 or 3.2 + plugin, where we can ask
for only what we need, but for older bugtrackers we ask for all the
data and then parse it, because there's no other option.

>> I'll happily rename the field, though.
>
> Thanks. You might want to hold off until stub & Bjorn have their say though.
>

Okay.

--
Graham Binns | PGP Key: EC66FA7D

[Stuart Bishop (stub)]

Fine. patch-2207-42-0.sql

I'm happy with 'next_check' as a column name. This is a repeating job. Jobs in the Jobs table run once and have a scheduled_start, date_started and date_finished timestamp.

review: Approve (db)

[Björn Tillenius (bjornt)]

< BjornT> gmb: sure, i'll look at it, if you explain how it will be used.
          in more detail :)
< gmb> BjornT: Cool. So, we want to be a bit more sensible with.
       scheduling bug watch checks. At the moment we check each watch.
       once every 24 hours, but if it's erroring a lot (which we now.
       record in the BugWatchActivity table) we don't want to check it so.
       often.
< gmb> BjornT: So, instead of building that logic into.
       BugTracker.getBugWatchesNeedingUpdate(), which would be complex at.
       best, we think we should have a garbo-hourly process that looks at.
       all the bug watches checked in the last hour, checks to see how.
       often they've errored and then schedules their next_check.
       accordingly.
< gmb> So BugTracker.getBugWatchesNeedingUpdate() will look at the.
       next_check field instead of lastchecked to determine whether a.
       watch needs checking.
< gmb> The advantage is that we don't have to further complicate.
       checkwatches to get the back-off for erroring watches.
< BjornT> gmb: ok, that makes sense. i'm wondering if we could find a.
          better name? if you set next_check to some time, it doesn't.
          mean that it will be checked exactly at that time, will it?
< gmb> BjornT: Hmm, true. How about check_after?
< stub> check_due ?
 * stub gets out his bike shed schematics
< gmb> It should be blue.
< BjornT> gmb: maybe next_check is good, as long as there is a comment.
          explaining the semantics. can't think of a better name, and i.
          think next_check is better than the other suggestions.
< gmb> BjornT: Right; I'll add the comment (should've added it anyway).

review: Approve (db)

[Jonathan Lange (jml)]

I can go either way.

review: Abstain (db)

=== modified file 'database/schema/comments.sql'
--- database/schema/comments.sql	2010-03-18 17:07:11 +0000
+++ database/schema/comments.sql	2010-03-23 15:43:33 +0000
@@ -319,6 +319,7 @@
 COMMENT ON COLUMN BugWatch.last_error_type IS 'The type of error which last prevented this entry from being updated. Legal values are defined by the BugWatchErrorType enumeration.';
 COMMENT ON COLUMN BugWatch.remote_importance IS 'The importance of the bug as returned by the remote server. This will be converted into a Launchpad BugTaskImportance value.';
 COMMENT ON COLUMN BugWatch.remote_lp_bug_id IS 'The bug in Launchpad that the remote bug is pointing at. This can be different than the BugWatch.bug column, since the same remote bug can be linked from multiple bugs in Launchpad, but the remote bug can only link to a single bug in Launchpad. The main use case for this column is to avoid having to query the remote bug tracker for this information, in order to decide whether we need to give this information to the remote bug tracker.';
+COMMENT ON COLUMN BugWatch.next_check IS 'The time after which the watch should next be checked. Note that this does not denote an exact schedule for the next check since checkwatches only runs periodically.';
 
 
 -- BugWatchActivity

=== added file 'database/schema/patch-2207-42-0.sql'
--- database/schema/patch-2207-42-0.sql	1970-01-01 00:00:00 +0000
+++ database/schema/patch-2207-42-0.sql	2010-03-23 15:43:33 +0000
@@ -0,0 +1,7 @@
+SET client_min_messages=ERROR;
+
+ALTER TABLE BugWatch ADD COLUMN next_check timestamp without time zone;
+
+CREATE INDEX bugwatch__next_check__idx ON BugWatch(next_check);
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 42, 0);

=== modified file 'lib/lp/bugs/doc/bug-watch-activity.txt'
--- lib/lp/bugs/doc/bug-watch-activity.txt	2010-03-19 23:28:34 +0000
+++ lib/lp/bugs/doc/bug-watch-activity.txt	2010-03-23 15:43:33 +0000
@@ -59,3 +59,86 @@
     None
     >>> print activity.oops_id
     None
+
+
+Recording BugWatch updates as BugWatchActivity
+-----------------------------------------------
+
+Whenever an update is made to a BugWatch that update is recorded as a
+BugWatchActivity entry.
+
+We can demonstrate this by passing our bug watch to
+BugWatchUpdater.updateBugWatches().
+
+    >>> from canonical.launchpad.scripts.logger import QuietFakeLogger
+    >>> from lp.bugs.scripts.checkwatches import BugWatchUpdater
+    >>> from lp.bugs.tests.externalbugtracker import TestExternalBugTracker
+    >>> updater = BugWatchUpdater(transaction, QuietFakeLogger())
+    >>> updater.updateBugWatches(
+    ...     TestExternalBugTracker('http://example.com'), [bug_watch])
+
+An extra activity item will have been added to the BugWatch's activity
+property.
+
+    >>> print bug_watch.activity.count()
+    2
+
+The most recent activity entry will have a result of None since it was
+successful.
+
+    >>> most_recent_activity = bug_watch.activity.first()
+    >>> print most_recent_activity.result
+    None
+
+Its message will also be empty
+
+    >>> print most_recent_activity.message
+    None
+
+As will its oops_id
+
+    >>> print most_recent_activity.oops_id
+    None
+
+If the remote bug tracker breaks during an update the error will be
+recorded in the activity entry for that update.
+
+    >>> from lp.bugs.externalbugtracker.base import BugNotFound
+    >>> from lp.bugs.tests.externalbugtracker import (
+    ...     TestBrokenExternalBugTracker)
+    >>> broken_bugtracker = TestBrokenExternalBugTracker('http://example.com')
+    >>> broken_bugtracker.get_remote_status_error = BugNotFound
+
+    >>> updater.updateBugWatches(broken_bugtracker, [bug_watch])
+
+Another entry will have been added to the watch's activity property.
+
+    >>> print bug_watch.activity.count()
+    3
+
+And this time its result field will record that the the remote bug was
+not found.
+
+    >>> most_recent_activity = bug_watch.activity.first()
+    >>> print most_recent_activity.result.title
+    Bug Not Found
+
+The OOPS ID for the error will also have been recorded.
+
+    >>> print most_recent_activity.oops_id
+    OOPS...
+
+The BugWatchUpdater also adds BugWatchActivity entries when errors occur
+that don't have an entry in the BugWatchActivityStatus DB Enum.
+
+    >>> broken_bugtracker.get_remote_status_error = Exception
+    >>> updater.updateBugWatches(broken_bugtracker, [bug_watch])
+    >>> most_recent_activity = bug_watch.activity.first()
+
+    >>> print most_recent_activity.result.title
+    Unknown
+
+The OOPS ID of the error is recorded so that we can debug it.
+
+    >>> print most_recent_activity.oops_id
+    OOPS...

=== modified file 'lib/lp/bugs/model/bugwatch.py'
--- lib/lp/bugs/model/bugwatch.py	2010-03-22 10:36:53 +0000
+++ lib/lp/bugs/model/bugwatch.py	2010-03-23 15:43:33 +0000
@@ -282,8 +282,10 @@
         activity = BugWatchActivity()
         activity.bug_watch = self
         activity.result = result
-        activity.message = message
-        activity.oops_id = oops_id
+        if message is not None:
+            activity.message = unicode(message)
+        if oops_id is not None:
+            activity.oops_id = unicode(oops_id)
         store = IStore(BugWatchActivity)
         store.add(activity)
 
@@ -292,7 +294,8 @@
         store = Store.of(self)
         return store.find(
             BugWatchActivity,
-            BugWatchActivity.bug_watch == self).order_by('activity_date')
+            BugWatchActivity.bug_watch == self).order_by(
+                Desc('activity_date'))
 
 
 class BugWatchSet(BugSetBase):

=== modified file 'lib/lp/bugs/scripts/checkwatches.py'
--- lib/lp/bugs/scripts/checkwatches.py	2010-03-23 03:14:13 +0000
+++ lib/lp/bugs/scripts/checkwatches.py	2010-03-23 15:43:33 +0000
@@ -826,6 +826,7 @@
                 new_remote_importance = None
                 new_malone_importance = None
                 error = None
+                oops_id = None
 
                 # XXX: 2007-10-17 Graham Binns
                 #      This nested set of try:excepts isn't really
@@ -846,7 +847,7 @@
                     error = get_bugwatcherrortype_for_error(ex)
                     message = error_type_messages.get(
                         error, error_type_message_default)
-                    self.warning(
+                    oops_id = self.warning(
                         message % {
                             'bug_id': remote_bug_id,
                             'base_url': remotesystem.baseurl,
@@ -879,6 +880,7 @@
                             self.pushBugComments(remotesystem, bug_watch)
                         if ISupportsBackLinking.providedBy(remotesystem):
                             self.linkLaunchpadBug(remotesystem, bug_watch)
+                    bug_watch.addActivity(result=error, oops_id=oops_id)
 
             except (KeyboardInterrupt, SystemExit):
                 # We should never catch KeyboardInterrupt or SystemExit.
@@ -888,7 +890,7 @@
                 # Restart transaction before recording the error.
                 self.txn.abort()
                 # Send the error to the log.
-                self.error(
+                oops_id = self.error(
                     "Failure updating bug %r on %s (local bugs: %s)." %
                             (remote_bug_id, bug_tracker_url, local_ids),
                     properties=[
@@ -901,9 +903,10 @@
                 # their lastchecked dates so that we don't try to
                 # re-check them every time checkwatches runs.
                 errortype = get_bugwatcherrortype_for_error(error)
-                for bugwatch in bug_watches:
-                    bugwatch.lastchecked = UTC_NOW
-                    bugwatch.last_error_type = errortype
+                for bug_watch in bug_watches:
+                    bug_watch.lastchecked = UTC_NOW
+                    bug_watch.last_error_type = errortype
+                    bug_watch.addActivity(result=errortype, oops_id=oops_id)
                 # We need to commit the transaction, in case the next
                 # bug fails to update as well.
                 self.txn.commit()
@@ -1156,6 +1159,10 @@
         # Also put it in the log.
         self.log.warning("%s (%s)" % (message, oops_info.oopsid))
 
+        # Return the OOPS ID so that we can use it in
+        # BugWatchActivity.
+        return oops_info.oopsid
+
     def error(self, message, properties=None, info=None):
         """Record an error related to this external bug tracker."""
         oops_info = report_oops(message, properties, info)
@@ -1163,6 +1170,10 @@
         # Also put it in the log.
         self.log.error("%s (%s)" % (message, oops_info.oopsid))
 
+        # Return the OOPS ID so that we can use it in
+        # BugWatchActivity.
+        return oops_info.oopsid
+
 
 class BaseScheduler:
     """Run jobs according to a policy."""