On Thu, May 27, 2010 at 05:13:27AM -0000, Stuart Bishop wrote:
> Review: Needs Fixing db
> On Wed, May 26, 2010 at 10:26 PM, Graham Binns <email address hidden> wrote:
> 
> > === added file 'database/schema/patch-2207-99-0.sql'
> 
> There is no need for a database patch - the stored procedure should be added to trusted.sql (which is becoming unwieldy - I think I need to split it up).

Righto, I've moved it.

>[snip]
> 
> You can do this in a single query rather than one per task:
> 
> SELECT MAX(GREATEST(Product.max_bug_heat, Distribution.max_bug_heat))
> FROM BugTask
> LEFT OUTER JOIN ProductSeries ON BugTask.productseries = ProductSeries.id
> LEFT OUTER JOIN Product ON (
>     BugTask.product = Product.id
>     OR ProductSeries.product = Product.id)
> LEFT OUTER JOIN DistroSeries ON BugTask.distroseries = DistroSeries.id
> LEFT OUTER JOIN Distribution ON (
>     BugTask.distribution = Distribution.id
>     OR DistroSeries.distribution = Distribution.id)
> WHERE
>     BugTask.bug = 1
> 

Sweet; fixed.

> > +    # It would be nice to be able to just SELECT * here, but we need to
> > +    # format the timestamps so that datetime.strptime() won't choke on
> > +    # them.
> 
> Using epoch time would avoid the need for parsing strings (use
> 'EXTRACT(epoch FROM datecreated)' instead of 'TO_CHAR(...)').  It
> probably doesn't save any significant time though.

I've used epoch time instead (see below).

> 
> > +    if bug_data.nrows() == 0:
> > +        return 0
> 
> I'd return None or raise an exception here - the bug doesn't exist so
> the heat is undefined.

I've gone for raising an exception; is it worth defining an exception
subclass to use here or is Exception() okay?

> > +    bug = bug_data[0]
> > +    if bug['duplicateof'] is not None:
> > +        return 0
> 
> Again, None or the heat of the master bug. We should catch cases where
> we are attempting to calculate the bug heat of non-existent or
> duplicate bugs.
> 

Okay. I'll update Bug.updateHeat() so that it doesn't try calling this
function if the bug is a duplicate and have this return None.

> > +    if bug['private']:
> > +        heat['privacy'] = BugHeatConstants.PRIVACY
> > +    if bug['security_related']:
> > +        heat['security'] = BugHeatConstants.SECURITY
> > +
> > +    # Get the heat from subscribers.
> > +    sub_count = plpy.execute("""
> > +        SELECT bug, COUNT(id) AS sub_count
> > +            FROM BugSubscription
> > +            WHERE bug = %s
> > +            GROUP BY bug
> > +            """ % bug_id)
> 
> This is unnecessarily complex. The simpler query is just "SELECT COUNT(*) FROM BugSubscription WHERE bug=%s".
> 

Right; this is because I ripped it off from the query we were using to
update the bugs' heat manually :). Fixed now.

> > +    if sub_count.nrows() > 0:
> > +        heat['subscribers'] = (
> > +            BugHeatConstants.SUBSCRIBER * sub_count[0]['sub_count'])
> 
> You don't need the nrows() guard, as the query will always return one row.

Ok.

> > +    # Get the heat from subscribers via duplicates.
> > +    subs_from_dupes = plpy.execute("""
> > +        SELECT bug.duplicateof AS dupe_of,
> > +                COUNT(bugsubscription.id) AS dupe_sub_count
> > +        FROM bugsubscription, bug
> > +        WHERE   bug.duplicateof = %s
> > +            AND Bugsubscription.bug = bug.id
> > +        GROUP BY Bug.duplicateof""" % bug_id)
> > +    if subs_from_dupes.nrows() > 0:
> > +        heat['subcribers_from_dupes'] = (
> > +            BugHeatConstants.SUBSCRIBER * sub_count[0]['dupe_sub_count'])
> 
> Similar here. nrows() guard is unnecessary. The query is also overly complex:
> 
> SELECT COUNT(*) FROM BugSubscription, Bug WHERE Bug.duplicateof = %s AND BugSubscription.bug = Bug.id
> 
> I notice that if a person is subscribed to both a duplicate and the master job, they are counted twice. If this is not correct, we can collapse the above two sections into one using the query:
> 
> SELECT COUNT(DISTINCT BugSubscription.person)
> FROM BugSubscription, Bug
> WHERE Bug.id = BugSubscription.bug
>     AND (Bug.id = %s OR Bug.duplicateof = %s)

Thanks, I'll use this.

> If you want to expand teams to get the genuine number of people subscribed:
> 
> SELECT COUNT(DISTINCT TeamParticipation.person)
> FROM TeamParticipation, BugSubscription, Bug
> WHERE
>     TeamParticipation.team = BugSubscription.person
>     AND Bug.id = BugSubscription.bug
>     AND (Bug.id = %s OR Bug.duplicateof = %s)
> 
> 
> Bug #1 for instance has 313 subscribers without expanding the teams,
> or 678 if you do expand the teams.

Having talked with Deryck, we don't want to do this; it would inflate
the heat counts by far too much.

> > +    # Bugs decay over time. Every day the bug isn't touched its heat
> > +    # decreases by 1%.
> > +    pg_datetime_fmt = "%Y-%m-%d %H:%M:%S"
> > +    date_last_updated = datetime.strptime(
> > +        bug['formatted_date_last_updated'], pg_datetime_fmt)
> > +    days_since_last_update = (datetime.utcnow() - date_last_updated).days
> > +    total_heat = int(total_heat * (0.99 ** days_since_last_update))
> 
> Yer... this would be simpler and clearer using epoch times.

Right, I've converted it to use epoch time and fromtimestamp().

> > +INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 99, 0);
> 
> As before, this should go into trusted.sql rather than a DB patch. We get to treat stored procedures as any other code, unlike database patches which cannot be reverted once applied.

Right.

> > -Sometimes, when a bug changes, we want to see the changes reflected in the bug's
> > -heat value immidiately, without waiting for heat to be recalculated. Currently
> > -we adjust heat immidiately for bug privacy and security.
> 
> immediately, not immidiately.

I've already fixed that (you're reading the removal part of the diff
;)).

> > +    def updateHeat(self):
> > +        """See `IBug`."""
> > +        # We need to flush the store first to ensure that changes are
> > +        # reflected in the new bug heat total.
> > +        store = Store.of(self)
> > +        store.flush()
> 
> You sure about that? Storm should be applying updates in order, so
> when calculate_bug_heat is actually executed (at commit time or when
> heat is next accessed) changes will be implicitly flushed.

Hmm. I put the flush() in there because in testing it seemed like heat
lagged behind where it should be. If I remove this, I get this result:

    >>> bug = factory.makeBug()
    >>> bug.heat
    8

    >>> changed = bug.setPrivate(True, bug.owner)
    >>> bug.heat
    8

    >>> changed = bug.setSecurityRelated(True, bug.owner)
    >>> bug.heat
    158

Where what I should be seeing is 

    >>> bug = factory.makeBug()
    >>> bug.heat
    8

    >>> changed = bug.setPrivate(True, bug.owner)
    >>> bug.heat
    158

    >>> changed = bug.setSecurityRelated(True, bug.owner)
    >>> bug.heat
    408

So I think this is definitely necessary (or maybe it's just necessary
for testing; I don't know. What are your recommendations?)