The default bug view has changed. See this FAQ.

Match on a list of locales, rather than exact only

RESOLVED FIXED

Status

Release Engineering
Balrog: Backend
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: nthomas, Assigned: bhearsum)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
I think I'm going to need to give a list of locales, eg
  ab,cd-EF,gh
for bug 657789, and don't want to write a rule for every locale.
(Reporter)

Comment 1

3 years ago
Created attachment 8340976 [details] [diff] [review]
First cut

I'm interested in what you think of this, as I can't say all the consequences are clear to me yet.
Attachment #8340976 - Flags: feedback?(bhearsum)
(Assignee)

Comment 2

3 years ago
Comment on attachment 8340976 [details] [diff] [review]
First cut

Review of attachment 8340976 [details] [diff] [review]:
-----------------------------------------------------------------

This looks sensible to me. Might be worth running compare-update-requests.py against aus4.m.o either locally or after you push this to dev.

We now have 4 different methods like this (version, channel, osVersion, locale) that are all very similar. It sets off my refactor alert...but these all of different business logic, so maybe it's probably fine how it is.
Attachment #8340976 - Flags: feedback?(bhearsum) → feedback+
(Reporter)

Updated

3 years ago
No longer blocks: 657789
(Assignee)

Updated

3 years ago
Duplicate of this bug: 933433
(Reporter)

Comment 4

2 years ago
Haven't got to this, being honest and unassigning.
Assignee: nthomas → nobody
(Assignee)

Updated

2 years ago
Blocks: 1134471
(Assignee)

Comment 5

2 years ago
I want this to make bug 1134471 easier, I can probably drive it to completion....
Assignee: nobody → bhearsum
(Assignee)

Comment 6

2 years ago
Created attachment 8576835 [details] [diff] [review]
increase locale column size

I discovered that the column wasn't wide enough when I tried out your patch in vagrant. This should increase it to 200 + adjust the form verification to accept that.

With this applied, I was able to create these rules:
mysql> select mapping, product, channel, locale from rules where channel="release";
+---------------------+-------------+---------+----------------+
| mapping             | product     | channel | locale         |
+---------------------+-------------+---------+----------------+
| Firefox-31.0-build2 | Firefox     | release | NULL           |
| Firefox-31.0-build1 | Firefox     | release | de,ru,zh-TW,zu |
+---------------------+-------------+---------+----------------+


And I get responses like:
(balrog)➜  balrog git:(localeslist) curl http://balrog.mozilla.dev:9000/update/3/Firefox/25.0/20120222174716/WINNT_x86-msvc/de/release/default/default/default/update.xml
<?xml version="1.0"?>
<updates>
    <update type="minor" displayVersion="31.0" appVersion="31.0" platformVersion="31.0" buildID="20140714151536" detailsURL="https://www.mozilla.org/de/firefox/31.0/releasenotes/">
        <patch type="complete" URL="http://download.mozilla.org/?product=Firefox-31.0-Complete&amp;os=win&amp;lang=de" hashFunction="sha512" hashValue="e472c908919f1e6a72ef05ba59b1b18dcdeae2c832c595d10a1c6812d5616ae2d05bbeb11ac587a9990166fc558f09ebedf7e9930cee3459848bb18d9ef22b67" size="39203770"/>
    </update>
</updates>%
(balrog)➜  balrog git:(localeslist) curl http://balrog.mozilla.dev:9000/update/3/Firefox/25.0/20120222174716/WINNT_x86-msvc/en-US/release/default/default/default/update.xml
<?xml version="1.0"?>
<updates>
    <update type="minor" displayVersion="31.0" appVersion="31.0" platformVersion="31.0" buildID="20140716183446" detailsURL="https://www.mozilla.org/en-US/firefox/31.0/releasenotes/">
        <patch type="complete" URL="http://download.mozilla.org/?product=Firefox-31.0-Complete&amp;os=win&amp;lang=en-US" hashFunction="sha512" hashValue="cd568f5155888e478bb38329292fc341759cd6d799ecb1f03c66d2b4c2e6b1aaa33eaf0322d97249b961e460e41dabb968effc7a3f1c539ee303aee9427eb696" size="39484922"/>
    </update>
</updates>%
Attachment #8340976 - Attachment is obsolete: true
Attachment #8576835 - Flags: review?(nthomas)
(Reporter)

Comment 7

2 years ago
Comment on attachment 8576835 [details] [diff] [review]
increase locale column size

Review of attachment 8576835 [details] [diff] [review]:
-----------------------------------------------------------------

::: auslib/db.py
@@ +707,5 @@
>  
> +    def _localeMatchesRule(self, ruleLocales, queryLocales):
> +        """Decides if a comma seperated list of locales in a rule matches an
> +        update request"""
> +        return self._matchesList(ruleLocales, queryLocales)

Nit, just the singular queryLocale instead of the plural.

@@ -713,5 @@
>             fallback, fallbackChannel should match the channel from the query."""
>          where=[
>              ((self.product==updateQuery['product']) | (self.product==None)) &
>              ((self.buildTarget==updateQuery['buildTarget']) | (self.buildTarget==None)) &
> -            ((self.locale==updateQuery['locale']) | (self.locale==None)) &

These queries get cheaper every time we add more functionality!

::: auslib/migrate/versions/007_longer_locales.py
@@ +7,5 @@
> +
> +def downgrade(migrate_engine):
> +    metadata = MetaData(bind=migrate_engine)
> +    Table('rules', metadata, autoload=True).c.locale.alter(type=String(10))
> +    Table('rules_history', metadata, autoload=True).c.locale.alter(type=String(10))

Do these work OK if you have longer strings in the DB, truncate or error ? Or is it only meant to work for short-term rollback where no rules have been added ?
Attachment #8576835 - Flags: review?(nthomas) → review+
(Assignee)

Comment 8

2 years ago
(In reply to Nick Thomas [:nthomas] from comment #7)
> @@ -713,5 @@
> >             fallback, fallbackChannel should match the channel from the query."""
> >          where=[
> >              ((self.product==updateQuery['product']) | (self.product==None)) &
> >              ((self.buildTarget==updateQuery['buildTarget']) | (self.buildTarget==None)) &
> > -            ((self.locale==updateQuery['locale']) | (self.locale==None)) &
> 
> These queries get cheaper every time we add more functionality!

:). If they do end up become a bottleneck or too expensive rule caching would help a lot. Some of them can probably be implemented in SQL if we try hard enough too.

> ::: auslib/migrate/versions/007_longer_locales.py
> @@ +7,5 @@
> > +
> > +def downgrade(migrate_engine):
> > +    metadata = MetaData(bind=migrate_engine)
> > +    Table('rules', metadata, autoload=True).c.locale.alter(type=String(10))
> > +    Table('rules_history', metadata, autoload=True).c.locale.alter(type=String(10))
> 
> Do these work OK if you have longer strings in the DB, truncate or error ?
> Or is it only meant to work for short-term rollback where no rules have been
> added ?

Looks like we truncate:
/usr/lib64/python2.6/site-packages/sqlalchemy/engine/default.py:325: Warning: Data truncated for column 'locale' at row 45
  cursor.execute(statement, parameters)

It really is just for short term rollback, but it would be nice if we could not silently truncate data. This is similar to bug 702271.
(Assignee)

Comment 9

2 years ago
Created attachment 8577302 [details] [diff] [review]
fix arg naming, don't truncate!

SQLAlchemy doesn't make it terribly easy to change the sql mode to something more restrictive. The underlying mysql driver supports setting it, but SQLAlchemy doesn't know how to forward to it along. I found this solution, which seems to work:
sqlalchemy.exc.DataError: (DataError) (1406, "Data too long for column 'locale' at row 45") '\nALTER TABLE rules CHANGE COLUMN locale locale VARCHAR(10)' ()

I left it off for everything except the database migration script. We probably need to do a lot of testing and maybe start catching these tracebacks if we want to enable it in production. That's better done elsewhere.
Attachment #8576835 - Attachment is obsolete: true
Attachment #8577302 - Flags: review?(nthomas)
(Reporter)

Comment 10

2 years ago
Comment on attachment 8577302 [details] [diff] [review]
fix arg naming, don't truncate!

Review of attachment 8577302 [details] [diff] [review]:
-----------------------------------------------------------------

Very sneaky that you only enable traditional mode for the schema changes.
Attachment #8577302 - Flags: review?(nthomas) → review+

Comment 11

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

https://github.com/mozilla/balrog/commit/d15f6cdfa23665c91021ff4b27e60a69e6b4b065
bug 945179: Match on a list of locales, rather than exact only. r=nthomas
(Assignee)

Comment 12

2 years ago
Comment on attachment 8577302 [details] [diff] [review]
fix arg naming, don't truncate!

(In reply to Nick Thomas [:nthomas] from comment #10)
> Comment on attachment 8577302 [details] [diff] [review]
> fix arg naming, don't truncate!
> 
> Review of attachment 8577302 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Very sneaky that you only enable traditional mode for the schema changes.

Yeah. If we didn't have the release channel in production I'd probably be OK flipping the switch everywhere. Feels too risky to do on a whim now though. I think bug 702271 will be a good place to do that.
Attachment #8577302 - Flags: checked-in+
(Assignee)

Comment 13

2 years ago
Planning to push this and bug 1141513 to prod at the same time, provided that's ready prior to RC.
(Assignee)

Updated

2 years ago
Depends on: 1145144
(Assignee)

Comment 14

2 years ago
This was pushed to prod today, and is working. I did some testing of it when I set-up bug 1134471.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.