1225793 - Autoland MozreviewUpdate table should be normalized

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Mauro Doglio [:mdoglio]]

MozreviewUpdate contains the outgoing notification to mozreview and their associated request ids.
There are only 3 columns on that table:
- request_id
- pingback_url
- data text

Since we can have more than one notification per autoland request we should split this table in two: one for the autoland request (containing request_id and pingback_url) and one for the notification data.

Comment 1

[Mauro Doglio [:mdoglio]]

Oh wait, does MozreviewUpdate.request_id refer to Transplant.id? If that's the case we should:
- rename request_id to transplant_id
- define request_id as a foreign key
- move pingback_url to Transplant

Comment 2

[Dan Minor [:dminor]]

Attachment: MozReview Request: autoland: normalize mozreviewupdate table (bug 1225793) r=mdoglio

autoland: normalize mozreviewupdate table (bug 1225793) r=mdoglio

Comment 3

[Mauro Doglio [:mdoglio]]

Comment on attachment 8689517 [details]
MozReview Request: autoland: normalize mozreviewupdate table (bug 1225793) r=mdoglio

https://reviewboard.mozilla.org/r/25653/#review23057

::: autoland/autoland/autoland.py:169
(Diff revision 1)
> -            insert into MozreviewUpdate(request_id,pingback_url,data)
> +            insert into MozreviewUpdate(transplant_id,data)

This is very scary. I would get rid of the github pull request table first.

::: autoland/autoland/autoland.py:362
(Diff revision 1)
> -        select request_id,pingback_url,data
> +        select transplant_id,request,data

Once you add an autoincrement id as primary key on MozreviewUpdate, you need to retrieve that instead of transplant id.

::: autoland/autoland/autoland.py:363
(Diff revision 1)
> -        from MozreviewUpdate
> +        from MozreviewUpdate, Transplant

What you really want to do here is probably either a left join or an inner join between Transplant and MozreviewUpdate. I believe the query plan for a cartesian product is slightly worse, even though the result of the query would be the same.

::: autoland/autoland/autoland.py:386
(Diff revision 1)
> -                updated.append([request_id])
> +                updated.append([transplant_id])

`updated` should refer to the rows of MozreviewUpdate using the primary key instead of transplant_id.

::: autoland/autoland/autoland.py:397
(Diff revision 1)
> -            where request_id=%s
> +            where transplant_id=%s

Here you should delete by primary key and not by transplant_id. There could be more than one event to notify to mozreview for the same transplant request and we can't miss any of them!

::: autoland/sql/schema.sql:34
(Diff revision 1)
>  create table MozreviewUpdate (\

You also need an autoincrement primary key on this table to identify its rows.

Comment 4

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/25653/#review23057

> This is very scary. I would get rid of the github pull request table first.

I think it would make sense to remove the github pullrequest code until we're going to use it but every time I bring up the subject I'm told no :)
Instead I'll leave this code broken.

Comment 5

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/25653/#review23057

> What you really want to do here is probably either a left join or an inner join between Transplant and MozreviewUpdate. I believe the query plan for a cartesian product is slightly worse, even though the result of the query would be the same.

My reading of the postgres docs is that an inner join is implicit with the syntax I used, but I have no problems with making it explicit.

Comment 6

[Dan Minor [:dminor]]

Comment on attachment 8689517 [details]
MozReview Request: autoland: normalize mozreviewupdate table (bug 1225793) r=mdoglio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25653/diff/1-2/

Comment 7

[Mauro Doglio [:mdoglio]]

Comment on attachment 8689517 [details]
MozReview Request: autoland: normalize mozreviewupdate table (bug 1225793) r=mdoglio

https://reviewboard.mozilla.org/r/25653/#review23203

Comment 8

[Dan Minor [:dminor]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/1de17d1fb561