1107781 - Remove the call log db upgrade code

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

The call log db currently contains a long and winding update procedure to reach schema version 4. This procedure is complex and slow as well as unnecessary since no phone shipped with a database with a schema version prior to 4. The only use-case that would be affected by this are the reference workloads.

Removing that code would greatly reduce complexity of the call log DB. We should however upgrade the reference workloads so that they don't need to go through the upgrade process anymore.

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH WIP] Call log db machete attack

300+ lines slashed. This removes the migration logic for the data in the database but leaves the schema migration intact. I've also removed the progress-bar and it's associated code because it's also not needed anymore and we don't want to keep that extra complexity around (the fact that we needed a progress bar to upgrade the call log DB is also crazy!). I haven't upgraded the reference workloads yet, that'll be coming in the next patch.

Comment 2

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8532390 [details] [diff] [review]
[PATCH WIP] Call log db machete attack

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

Looks reasonable. As mentioned offline, we can hack out more than this.

Comment 3

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 2/3] Remove the upgrade procedure from versions 1 to 5 of the call log database

This is the first patch, chopping out the upgrade procedure, now with a more reasonable description :) This is patch 2/3 because I want the first patch in the series to be the reference workload upgrade so that in no version the workloads are broken.

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 3/3] Remove the unused dialerRecents store and all associated code, adjust the unit-tests accordingly

This was called "Machete - part 2" before I could come up with a better name. It chops out the unused dialerRecents object store plus all its associated methods and unit-tests. This also reworks the unit-tests so that they have proper setup()s and the database is cleared between each one. Redundant checks are also removed. I'm considering splitting the later part in two for making review easier.

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8533953 [details] [diff] [review]
[PATCH 2/3] Remove the upgrade procedure from versions 1 to 5 of the call log database

Obsoleting, I'll replace the patches here with a PR.

Comment 6

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8533955 [details] [diff] [review]
[PATCH 3/3] Remove the unused dialerRecents store and all associated code, adjust the unit-tests accordingly

Likewise.

Comment 7

[GitHub Autolander Bot]

Attachment: [gaia] gabrielesvelto:bug-1107781-call-log-machete-attack > mozilla-b2g:master

Comment 8

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8591734 [details] [review]
[gaia] gabrielesvelto:bug-1107781-call-log-machete-attack > mozilla-b2g:master

After some further wrestling with the reference workloads the call log database upgrade code is finally ready to go; r? Jon for the reference workloads and Doug for the dialer parts.

The PR is split in three patches:

- The first one upgrades the call log reference workloads to version 5 of the database and adds a couple of other empty reference workloads so that the reference-workload-empty target works out-of-the-box. These reference workloads will work on any device starting with the early 1.0.0 ones so we shouldn't have backward compatibility problems even if we use them on really old branches.

- The second patch chops out the upgrade procedure; it also removes the upgrade progress overlay which is now useless since upgrades won't take minutes anymore.

- The third patch removes the dialerRecents store which has been unused for a while and bumps the database version to 6. All the related dead code is removed and the related unit-tests have also been removed. This also reworks the unit-tests so that they have proper setup()s and the database is cleared between each one. Redundant checks are also removed; this makes the patch unfortunately large (even though it's still made mostly of deletions) but it's overdue IMHO. The unit-tests were in just as bad shape as the rest of the code, padded with redundant checks and often relying on the result of the previous test to work since the database wasn't cleared between them.

All in all I'm really happy to see this code go. It took me a while to properly migrate all the reference workloads because the upgrade code was - surprise, surprise - non-atomic and spawned transactions from within transactions which would then fail more-or-less randomly. The larger the call log the more likely the failure. It took me four tries to get the x-heavy reference workload properly migrated.

Comment 9

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8591734 [details] [review]
[gaia] gabrielesvelto:bug-1107781-call-log-machete-attack > mozilla-b2g:master

Tamara, could you take a look at this as well? I'll review once you have.

Comment 10

[Tamara Hills [:thills]]

Comment on attachment 8591734 [details] [review]
[gaia] gabrielesvelto:bug-1107781-call-log-machete-attack > mozilla-b2g:master

Hi Gabriele,

It looks good. So nice to get rid of all that!  I think what you did with the tests in call_log_db.js is a big improvement... much simpler.

I did very basic testing on the call log and also ran the tests.

Thanks,

-tamara

Comment 11

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8591734 [details] [review]
[gaia] gabrielesvelto:bug-1107781-call-log-machete-attack > mozilla-b2g:master

Thanks for the quick review Tamara, clearing Doug's r? flag.

Comment 12

[Jon Hylands [:jhylands]]

Comment on attachment 8591734 [details] [review]
[gaia] gabrielesvelto:bug-1107781-call-log-machete-attack > mozilla-b2g:master

Assuming that this works, the only real concern I have with reference workload related updates are that they maintain backwards compatibility with earlier versions of b2g/fxos. Since that appears to be the case here, I have no issues.

Comment 13

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Jon Hylands [:jhylands] from comment #12)
> Assuming that this works, the only real concern I have with reference
> workload related updates are that they maintain backwards compatibility with
> earlier versions of b2g/fxos. Since that appears to be the case here, I have
> no issues.

Yes, I've explicitly made sure that the upgraded workloads work with everything starting with v1.0.0. Thanks for the review, time to land.

Comment 14

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/b9667acfc50a7596346947b946f63a2351bfaf35