Last Comment Bug 782325 - Ensure IM conversations are reindexed when the gloda database is reset
: Ensure IM conversations are reindexed when the gloda database is reset
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: 15 Branch
: x86 All
-- normal (vote)
: Thunderbird 17.0
Assigned To: Mike Conley (:mconley)
Depends on:
  Show dependency treegraph
Reported: 2012-08-13 10:13 PDT by Mike Conley (:mconley)
Modified: 2012-08-20 07:17 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Patch v1 (7.84 KB, patch)
2012-08-14 11:16 PDT, Mike Conley (:mconley)
bugmail: review-
Details | Diff | Splinter Review
Patch v2 (10.67 KB, patch)
2012-08-14 12:20 PDT, Mike Conley (:mconley)
bugmail: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description User image Mike Conley (:mconley) 2012-08-13 10:13:20 PDT
We want to remove the JSON cache when the gloda database is nuked.
Comment 1 User image Andrew Sutherland [:asuth] 2012-08-13 14:50:49 PDT
(followup to IRC discussion:) Yes, since SQLite uses schema_version internally and mozStorage/gloda uses user_version, I think the preference is the easiest way to handle associating a generated id for the gloda database so that dependent data like this JSON cache can know when to be blown away.
Comment 2 User image Mike Conley (:mconley) 2012-08-14 11:16:29 PDT
Created attachment 651835 [details] [diff] [review]
Patch v1

This patch makes GlodaDatastore stash a UUID string in when the database is created, or if the GlodaDatastore is initted and no such pref value exists.

The IM indexer now stores this datastoreID in the JSON cache, and checks for it when doing the initial folder sweep. If the datastoreID from the JSON cache does not match the one that is stored in preferences, this means that the cache is no longer valid, so it is ignored and eventually overwritten.
Comment 3 User image Andrew Sutherland [:asuth] 2012-08-14 11:33:42 PDT
Comment on attachment 651835 [details] [diff] [review]
Patch v1

The logic is good and I am super-happy about your detailed comments, but I would strongly prefer that Gloda, GlodaDatastore, or GlodaIndexer be the interface to the uuid rather than having index_im access the pref directly.  Specifically I'm concerned that since it lives in a different folder and cargo culting is rife in our codebase and extensions that it's safest to have a more explicit interface.
Comment 4 User image Andrew Sutherland [:asuth] 2012-08-14 11:36:37 PDT
Also, I think you could add a unit test for this logic pretty easily to test_corrupt_database.js.  Specifically, set the preference to a made-up UUID at the start of the file before gloda gets bootstrapped.  Then, near the end of the test, check that the exposed variable is now a new UUID and also directly probe that the preference has that same value.
Comment 5 User image Mike Conley (:mconley) 2012-08-14 12:20:13 PDT
Created attachment 651851 [details] [diff] [review]
Patch v2

Thanks for the review, asuth!

Ok, the datastoreID is now accessed via a getter in Gloda, and the IM indexer has been updated accordingly. I've also augmented the current test in test_corrupt_database.js to check on our UUID regeneration behaviour.
Comment 6 User image Andrew Sutherland [:asuth] 2012-08-14 13:48:57 PDT
Comment on attachment 651851 [details] [diff] [review]
Patch v2

Awesome, thanks!  Bonus points for checking the new uuid length!  (Redeemable in snacks at your local Mozilla Space.)
Comment 7 User image Mike Conley (:mconley) 2012-08-14 13:51:42 PDT
Comment on attachment 651851 [details] [diff] [review]
Patch v2

Awesome - thanks for the reviews, asuth!
Comment 8 User image Mike Conley (:mconley) 2012-08-14 13:55:44 PDT

Note You need to log in before you can comment on or make changes to this bug.