Launchpad itself

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

  1. bugwatch-next_check-bug-544238
  2. Merge into db-devel
Proposed by Graham Binns
Status: Merged
Approved by: Graham Binns
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~gmb/launchpad/bugwatch-next_check-bug-544238
Merge into: lp:launchpad/db-devel
Diff against target: 215 lines (+113/-8)
5 files modified
database/schema/comments.sql (+1/-0)
database/schema/patch-2207-42-0.sql (+7/-0)
lib/lp/bugs/doc/bug-watch-activity.txt (+83/-0)
lib/lp/bugs/model/bugwatch.py (+6/-3)
lib/lp/bugs/scripts/checkwatches.py (+16/-5)
To merge this branch: bzr merge lp:~gmb/launchpad/bugwatch-next_check-bug-544238
Reviewer Review Type Date Requested Status
Jonathan Lange (community) db Abstain
Björn Tillenius (community) db Approve
Stuart Bishop (community) db Approve
Review via email: mp+21869@code.launchpad.net

Commit message

Add a next_check column to BugWatch.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Graham Binns (gmb) wrote :

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);

Revision history for this message
Jonathan Lange (jml) wrote :

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.

Revision history for this message
Graham Binns (gmb) 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.

I'll happily rename the field, though.

Revision history for this message
Jonathan Lange (jml) wrote :

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

Revision history for this message
Graham Binns (gmb) wrote :

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

Revision history for this message
Stuart Bishop (stub) wrote :

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)
Revision history for this message
Björn Tillenius (bjornt) wrote :

< 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)
Revision history for this message
Jonathan Lange (jml) wrote :

I can go either way.

review: Abstain (db)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
319COMMENT 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.';319COMMENT 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.';
320COMMENT 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.';320COMMENT 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.';
321COMMENT 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.';321COMMENT 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.';
322COMMENT 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.';
322323
323324
324-- BugWatchActivity325-- BugWatchActivity
325326
=== 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 @@
1SET client_min_messages=ERROR;
2
3ALTER TABLE BugWatch ADD COLUMN next_check timestamp without time zone;
4
5CREATE INDEX bugwatch__next_check__idx ON BugWatch(next_check);
6
7INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 42, 0);
08
=== 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 @@
59 None59 None
60 >>> print activity.oops_id60 >>> print activity.oops_id
61 None61 None
62
63
64Recording BugWatch updates as BugWatchActivity
65-----------------------------------------------
66
67Whenever an update is made to a BugWatch that update is recorded as a
68BugWatchActivity entry.
69
70We can demonstrate this by passing our bug watch to
71BugWatchUpdater.updateBugWatches().
72
73 >>> from canonical.launchpad.scripts.logger import QuietFakeLogger
74 >>> from lp.bugs.scripts.checkwatches import BugWatchUpdater
75 >>> from lp.bugs.tests.externalbugtracker import TestExternalBugTracker
76 >>> updater = BugWatchUpdater(transaction, QuietFakeLogger())
77 >>> updater.updateBugWatches(
78 ... TestExternalBugTracker('http://example.com'), [bug_watch])
79
80An extra activity item will have been added to the BugWatch's activity
81property.
82
83 >>> print bug_watch.activity.count()
84 2
85
86The most recent activity entry will have a result of None since it was
87successful.
88
89 >>> most_recent_activity = bug_watch.activity.first()
90 >>> print most_recent_activity.result
91 None
92
93Its message will also be empty
94
95 >>> print most_recent_activity.message
96 None
97
98As will its oops_id
99
100 >>> print most_recent_activity.oops_id
101 None
102
103If the remote bug tracker breaks during an update the error will be
104recorded in the activity entry for that update.
105
106 >>> from lp.bugs.externalbugtracker.base import BugNotFound
107 >>> from lp.bugs.tests.externalbugtracker import (
108 ... TestBrokenExternalBugTracker)
109 >>> broken_bugtracker = TestBrokenExternalBugTracker('http://example.com')
110 >>> broken_bugtracker.get_remote_status_error = BugNotFound
111
112 >>> updater.updateBugWatches(broken_bugtracker, [bug_watch])
113
114Another entry will have been added to the watch's activity property.
115
116 >>> print bug_watch.activity.count()
117 3
118
119And this time its result field will record that the the remote bug was
120not found.
121
122 >>> most_recent_activity = bug_watch.activity.first()
123 >>> print most_recent_activity.result.title
124 Bug Not Found
125
126The OOPS ID for the error will also have been recorded.
127
128 >>> print most_recent_activity.oops_id
129 OOPS...
130
131The BugWatchUpdater also adds BugWatchActivity entries when errors occur
132that don't have an entry in the BugWatchActivityStatus DB Enum.
133
134 >>> broken_bugtracker.get_remote_status_error = Exception
135 >>> updater.updateBugWatches(broken_bugtracker, [bug_watch])
136 >>> most_recent_activity = bug_watch.activity.first()
137
138 >>> print most_recent_activity.result.title
139 Unknown
140
141The OOPS ID of the error is recorded so that we can debug it.
142
143 >>> print most_recent_activity.oops_id
144 OOPS...
62145
=== 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 @@
282 activity = BugWatchActivity()282 activity = BugWatchActivity()
283 activity.bug_watch = self283 activity.bug_watch = self
284 activity.result = result284 activity.result = result
285 activity.message = message285 if message is not None:
286 activity.oops_id = oops_id286 activity.message = unicode(message)
287 if oops_id is not None:
288 activity.oops_id = unicode(oops_id)
287 store = IStore(BugWatchActivity)289 store = IStore(BugWatchActivity)
288 store.add(activity)290 store.add(activity)
289291
@@ -292,7 +294,8 @@
292 store = Store.of(self)294 store = Store.of(self)
293 return store.find(295 return store.find(
294 BugWatchActivity,296 BugWatchActivity,
295 BugWatchActivity.bug_watch == self).order_by('activity_date')297 BugWatchActivity.bug_watch == self).order_by(
298 Desc('activity_date'))
296299
297300
298class BugWatchSet(BugSetBase):301class BugWatchSet(BugSetBase):
299302
=== 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 @@
826 new_remote_importance = None826 new_remote_importance = None
827 new_malone_importance = None827 new_malone_importance = None
828 error = None828 error = None
829 oops_id = None
829830
830 # XXX: 2007-10-17 Graham Binns831 # XXX: 2007-10-17 Graham Binns
831 # This nested set of try:excepts isn't really832 # This nested set of try:excepts isn't really
@@ -846,7 +847,7 @@
846 error = get_bugwatcherrortype_for_error(ex)847 error = get_bugwatcherrortype_for_error(ex)
847 message = error_type_messages.get(848 message = error_type_messages.get(
848 error, error_type_message_default)849 error, error_type_message_default)
849 self.warning(850 oops_id = self.warning(
850 message % {851 message % {
851 'bug_id': remote_bug_id,852 'bug_id': remote_bug_id,
852 'base_url': remotesystem.baseurl,853 'base_url': remotesystem.baseurl,
@@ -879,6 +880,7 @@
879 self.pushBugComments(remotesystem, bug_watch)880 self.pushBugComments(remotesystem, bug_watch)
880 if ISupportsBackLinking.providedBy(remotesystem):881 if ISupportsBackLinking.providedBy(remotesystem):
881 self.linkLaunchpadBug(remotesystem, bug_watch)882 self.linkLaunchpadBug(remotesystem, bug_watch)
883 bug_watch.addActivity(result=error, oops_id=oops_id)
882884
883 except (KeyboardInterrupt, SystemExit):885 except (KeyboardInterrupt, SystemExit):
884 # We should never catch KeyboardInterrupt or SystemExit.886 # We should never catch KeyboardInterrupt or SystemExit.
@@ -888,7 +890,7 @@
888 # Restart transaction before recording the error.890 # Restart transaction before recording the error.
889 self.txn.abort()891 self.txn.abort()
890 # Send the error to the log.892 # Send the error to the log.
891 self.error(893 oops_id = self.error(
892 "Failure updating bug %r on %s (local bugs: %s)." %894 "Failure updating bug %r on %s (local bugs: %s)." %
893 (remote_bug_id, bug_tracker_url, local_ids),895 (remote_bug_id, bug_tracker_url, local_ids),
894 properties=[896 properties=[
@@ -901,9 +903,10 @@
901 # their lastchecked dates so that we don't try to903 # their lastchecked dates so that we don't try to
902 # re-check them every time checkwatches runs.904 # re-check them every time checkwatches runs.
903 errortype = get_bugwatcherrortype_for_error(error)905 errortype = get_bugwatcherrortype_for_error(error)
904 for bugwatch in bug_watches:906 for bug_watch in bug_watches:
905 bugwatch.lastchecked = UTC_NOW907 bug_watch.lastchecked = UTC_NOW
906 bugwatch.last_error_type = errortype908 bug_watch.last_error_type = errortype
909 bug_watch.addActivity(result=errortype, oops_id=oops_id)
907 # We need to commit the transaction, in case the next910 # We need to commit the transaction, in case the next
908 # bug fails to update as well.911 # bug fails to update as well.
909 self.txn.commit()912 self.txn.commit()
@@ -1156,6 +1159,10 @@
1156 # Also put it in the log.1159 # Also put it in the log.
1157 self.log.warning("%s (%s)" % (message, oops_info.oopsid))1160 self.log.warning("%s (%s)" % (message, oops_info.oopsid))
11581161
1162 # Return the OOPS ID so that we can use it in
1163 # BugWatchActivity.
1164 return oops_info.oopsid
1165
1159 def error(self, message, properties=None, info=None):1166 def error(self, message, properties=None, info=None):
1160 """Record an error related to this external bug tracker."""1167 """Record an error related to this external bug tracker."""
1161 oops_info = report_oops(message, properties, info)1168 oops_info = report_oops(message, properties, info)
@@ -1163,6 +1170,10 @@
1163 # Also put it in the log.1170 # Also put it in the log.
1164 self.log.error("%s (%s)" % (message, oops_info.oopsid))1171 self.log.error("%s (%s)" % (message, oops_info.oopsid))
11651172
1173 # Return the OOPS ID so that we can use it in
1174 # BugWatchActivity.
1175 return oops_info.oopsid
1176
11661177
1167class BaseScheduler:1178class BaseScheduler:
1168 """Run jobs according to a policy."""1179 """Run jobs according to a policy."""

Subscribers

People subscribed via source and target branches

to status/vote changes: