733274 - Rename throttle to backgroundRate in Balrog

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P3)

Description

[Nick Thomas [:nthomas] (UTC+12)]

'throttle' is ambiguous, eg is it a third open or a third closed ?. In bug 685862 I put off this rename until we figure out a db migration strategy (bug 733273).

Should also do update_type -> updateType in the Rules table, to match the camel case used for the other columns.

Comment 1

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: Patch from bug 685862

This is the part of bug 685862 which didn't land yet.

Comment 2

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to Nick Thomas [:nthomas] from comment #2)
> 'throttle' is ambiguous, eg is it a third open or a third closed ?. In bug
> 685862 I put off this rename until we figure out a db migration strategy
> (bug 733273).

Thanks for filing this Nick. Anything that makes this mechanism less ambiguous will be great - this keeps coming up in cross-group coordination meetings.

Comment 3

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

found in triage.

Comment 4

[[e-mail] (:bhearsum)]

Attachment: updated patch

Here's an updated patch for this, including a migration script. This was mostly sed written, with a couple of fixes for sillyness (test methods getting renamed to something with a space in them).

If we land like this we'll have a brief downtime until the db + code is fully in sync to use the new column. Since we're still only in dev I don't think it's worth it to try to avoid that. In the future we'll have to do two stage upgrades for things like this.

Comment 5

[[e-mail] (:bhearsum)]

Comment on attachment 765563 [details] [diff] [review]
updated patch

Hmm, while starting to look at bug 884563 I noticed that the CheckConstraint may break...need to looking into that more, removing review request for now.

Comment 6

[[e-mail] (:bhearsum)]

Attachment: drop check constraint

Turns out that check constraints don't work in MySQL anyways....http://docs.sqlalchemy.org/en/rel_0_7/core/schema.html#check-constraint

I asked around #db and there's some ways to implement them (triggers, updateable views) - but the general consensus is that it should be dealt with at the app level. This already gets enforced by the Form, so I think we should just drop the database constraint. This new patch does that.

Since these were never really supported, I've altered the original table revision rather than adding something to remove the non-existent constraint in the latest upgrade script.

Comment 7

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 765597 [details] [diff] [review]
drop check constraint

Looks good. I decided backgroundRate was fine, and if there's any confusion we can modify the UI side to clarify.

Comment 8

[[e-mail] (:bhearsum)]

Comment on attachment 765597 [details] [diff] [review]
drop check constraint

Going to wait until I see a web head update to this before I upgrade the db.

Comment 9

[[github robot]]

Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/bba8b9c8a92055ecd69a45bba38490755d4a464c
bug 733274: Rename throttle to backgroundRate in Balrog. r=nthomas

Comment 10

[[e-mail] (:bhearsum)]

This is landed, and the db has been upgraded. Looks fine to me, and tests passed: http://10.134.48.37:8080/job/Balrog/29/.