Closed Bug 1107781 Opened 6 years ago Closed 6 years ago

Remove the call log db upgrade code

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S10 (17apr)

People

(Reporter: gsvelto, Assigned: gsvelto)

References

Details

(Whiteboard: [planned-sprint c=3])

Attachments

(1 file, 3 obsolete files)

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.
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.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Attachment #8532390 - Flags: feedback?(drs.bugzilla)
Depends on: 1108008
Whiteboard: [planned-sprint c=3]
Target Milestone: --- → 2.2 S2 (19dec)
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.
Attachment #8532390 - Flags: feedback?(drs.bugzilla) → feedback+
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.
Attachment #8532390 - Attachment is obsolete: true
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.
Target Milestone: 2.2 S2 (19dec) → 2.2 S3 (9jan)
Target Milestone: 2.2 S3 (9jan) → 2.2 S7 (6mar)
Target Milestone: 2.2 S7 (6mar) → 2.2 S8 (20mar)
Target Milestone: 2.2 S8 (20mar) → 2.2 S9 (3apr)
Target Milestone: 2.2 S9 (3apr) → 2.2 S10 (17apr)
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.
Attachment #8533953 - Attachment is obsolete: true
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.
Attachment #8533955 - Attachment is obsolete: true
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.
Attachment #8591734 - Flags: review?(jhylands)
Attachment #8591734 - Flags: review?(drs)
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.
Attachment #8591734 - Flags: review?(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
Attachment #8591734 - Flags: review?(thills) → review+
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.
Attachment #8591734 - Flags: review?(drs)
Blocks: 1156189
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.
Attachment #8591734 - Flags: review?(jhylands) → review+
(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.
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.