Page MenuHomePhabricator

Replace custom saved pages cache with OkHttp cache
Closed, ResolvedPublic

Description

  • Add second OkHttp cache for saved pages independent of app cache files (cannot be deleted by clearing app cache)
    • Customize to allow pages to be marked as sticky (not needed)
    • Add as a second data source
  • Remove saved pages cache and update SavedPagesSyncService
  • Figure out what to do about Fresco's special cache and make a ticket
  • Create a ticket for updating the sync service to use the new headers and CacheDelegate.remove() method if it doesn't already exist

Develop and merge to separate branch as necessary

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 1 2017, 2:32 PM

@Mholloway, @Dbrant, we talked about this in our meeting but I don't think we ever created the task. Please update if I've missed something

Change 339574 had a related patch set uploaded (by Niedzielski):
Update: add second OkHttp cache for saved content

https://gerrit.wikimedia.org/r/339574

Niedzielski updated the task description. (Show Details)Feb 23 2017, 10:43 PM

Change 340433 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Hygiene: remove duplicate method in WidgetProviderFeaturedPage

https://gerrit.wikimedia.org/r/340433

Change 340563 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Hygiene: remove duplicate method in WidgetProviderFeaturedPage

https://gerrit.wikimedia.org/r/340563

Change 340676 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Hygiene: rename PageService classes to PageClients

https://gerrit.wikimedia.org/r/340676

Change 340677 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Hygiene: delete PageLoadStrategy

https://gerrit.wikimedia.org/r/340677

Change 340678 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Hygiene: rename PageDataClient to PageFragmentLoadState

https://gerrit.wikimedia.org/r/340678

Change 340680 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Hygiene: move MediaWiki fallback checks from clients to response interceptor

https://gerrit.wikimedia.org/r/340680

Change 340687 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Hygiene: clean up empty string checks

https://gerrit.wikimedia.org/r/340687

Change 340688 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Hygiene: move Wikipedia Zero header checks from clients to response interceptor

https://gerrit.wikimedia.org/r/340688

Change 340761 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Hygiene: isolate the Retrofit *PageClient services

https://gerrit.wikimedia.org/r/340761

Change 340762 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Hygiene: consolidate service caches into WikiCachedService

https://gerrit.wikimedia.org/r/340762

Change 340774 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Hygiene: hold WikiCachedService in clients, not implementation

https://gerrit.wikimedia.org/r/340774

Change 340834 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Hygiene: replace all Response.isSuccessful() calls with interceptor

https://gerrit.wikimedia.org/r/340834

Change 340563 merged by Niedzielski:
[apps/android/wikipedia] Hygiene: remove duplicate method in WidgetProviderFeaturedPage

https://gerrit.wikimedia.org/r/340563

Change 340904 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Hygiene: allow PageClients requestors to specify a/synchronicity

https://gerrit.wikimedia.org/r/340904

Change 340433 abandoned by Niedzielski:
Hygiene: remove duplicate method in WidgetProviderFeaturedPage

Reason:
merged to master yo!

https://gerrit.wikimedia.org/r/340433

Change 340906 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Update: add second OkHttp cache for saved content

https://gerrit.wikimedia.org/r/340906

Change 339574 abandoned by Niedzielski:
Update: add second OkHttp cache for saved content

Reason:
Moved to I45825777d18cbbc686d757a715002464cf94ae3f

https://gerrit.wikimedia.org/r/339574

Change 340676 merged by jenkins-bot:
[apps/android/wikipedia] Hygiene: rename PageService classes to PageClients

https://gerrit.wikimedia.org/r/340676

Change 340677 merged by jenkins-bot:
[apps/android/wikipedia] Hygiene: delete PageLoadStrategy

https://gerrit.wikimedia.org/r/340677

Change 340678 merged by jenkins-bot:
[apps/android/wikipedia] Hygiene: rename PageDataClient to PageFragmentLoadState

https://gerrit.wikimedia.org/r/340678

Change 340680 merged by jenkins-bot:
[apps/android/wikipedia] Hygiene: move MediaWiki fallback checks from clients to response interceptor

https://gerrit.wikimedia.org/r/340680

Change 340687 merged by jenkins-bot:
[apps/android/wikipedia] Hygiene: clean up empty string checks

https://gerrit.wikimedia.org/r/340687

Change 340688 merged by jenkins-bot:
[apps/android/wikipedia] Hygiene: move Wikipedia Zero header checks from clients to response interceptor

https://gerrit.wikimedia.org/r/340688

Change 340761 merged by jenkins-bot:
[apps/android/wikipedia] Hygiene: isolate the Retrofit *PageClient services

https://gerrit.wikimedia.org/r/340761

Change 340762 merged by jenkins-bot:
[apps/android/wikipedia] Hygiene: consolidate service caches into WikiCachedService

https://gerrit.wikimedia.org/r/340762

Change 340774 merged by jenkins-bot:
[apps/android/wikipedia] Hygiene: hold WikiCachedService in clients, not implementation

https://gerrit.wikimedia.org/r/340774

Change 340834 merged by jenkins-bot:
[apps/android/wikipedia] Hygiene: replace all Response.isSuccessful() calls with interceptor

https://gerrit.wikimedia.org/r/340834

Change 341386 had a related patch set uploaded (by niedzielski; owner: sniedzielski):
[apps/android/wikipedia] Remove page combo endpoint usage

https://gerrit.wikimedia.org/r/341386

Change 341387 had a related patch set uploaded (by niedzielski; owner: sniedzielski):
[apps/android/wikipedia] Add cache option to PageClient

https://gerrit.wikimedia.org/r/341387

Change 340904 merged by jenkins-bot:
[apps/android/wikipedia] Hygiene: allow PageClients requestors to specify a/synchronicity

https://gerrit.wikimedia.org/r/340904

Change 343287 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Hygiene: remove unused SavedPage methods and infer nullity

https://gerrit.wikimedia.org/r/343287

Change 343288 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Hygiene: rename ImageUrlMap to ImageUrlHtmlParser

https://gerrit.wikimedia.org/r/343288

Change 343289 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Update: remove saved page image downloading and link remapping

https://gerrit.wikimedia.org/r/343289

Change 343290 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Update: use the new save page download api

https://gerrit.wikimedia.org/r/343290

Change 343291 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Hygiene: add jsoup v1.10.2

https://gerrit.wikimedia.org/r/343291

Change 343292 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Update: prepare to replace image URL parser

https://gerrit.wikimedia.org/r/343292

Change 343293 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Update: expose WikiSite URI

https://gerrit.wikimedia.org/r/343293

Change 343294 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Update: use OkHttp for WebView requests

https://gerrit.wikimedia.org/r/343294

Change 343295 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Update: do not forbid Fresco image request caching

https://gerrit.wikimedia.org/r/343295

Change 343296 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia] Fix: save page downloading

https://gerrit.wikimedia.org/r/343296

I have a couple of questions about the relationship between the two caches that I'm not seeing clear answers to just from looking at the implementation.

  1. Can a page be in both the http and saved pages cache, or are they mutually exclusive?
  2. If a page is in both caches and fetched from the network while online, are both caches updated?
  3. What's the order of preference among { network, http_cache, saved_pages_cache } If the user is online and loads a page that's in the saved page cache?
  4. Is the reading list page database table updated when the saved pages cache is updated, so that getting the mtime for the page's database row will always result in an accurate last-modified time?

For context, I'm trying to figure out how T156082 should be done (particularly retrieving the last-modified timestamp), which is the motivating factor behind (4). The rest are just to firm up my understanding of how this works generally.

I'm still working out bugs but:

1

A saved page may exist in any combination of caches. Consider the following scenarios:

  1. [✓Saved cache] [✓ Network cache] A user marks a page as saved and both caches have space
  2. [✓Saved cache] [✗ Network cache] A user marks a page as saved, then clears the app cache
  3. [✗ Saved cache] [✓ Network cache] A rare but supported case that might occur because the disk cache is corrupted or the disk cache is set to be smaller than the app cache
  4. [✗Saved cache] [✗ Network cache] A user marks many pages as saved and one of them falls out of both LRU caches

A non-saved page may only appear in the network cache (no change in behavior).

2

There's been no change to how the default caching works. The network and network cache are always attempted when permitted by headers and request type. The saved page cache is also always attempted when permitted by the same headers and request type. For every network request, the saved page cache is only modified when permitted by the same headers and request type AND any of the following circumstances: 1) new: the custom X-Save header is true, 2) update: the page was previously saved. The saved page cache can also drop entries through an LRU disk overflow mechanism and a couple exposed APIs for removing and retrieving a cache entry.

3

The normal cache invalidation rules apply. The response returned is either: network only, updated network cached version, updated network / network cached saved cached version, saved cached version. The network cache is updated by network responses. The saved page cache is updated by network + network cached responses.

4

There hasn't been any change in this functionality except which APIs are used to download a page (lead + sections with X-Save header instead of pageCombo). The behavior I see before and after the patches is that any time a page is reloaded, the row is marked for update and the save page sync service runs which updates the saved page cache. Always use the database to get saved page status as the saved page cache can drop pages when disk space is exhausted.

Change 343295 merged by jenkins-bot:
[apps/android/wikipedia] Update: do not forbid Fresco image request caching

https://gerrit.wikimedia.org/r/343295

Change 343294 merged by jenkins-bot:
[apps/android/wikipedia] Update: use OkHttp for WebView requests

https://gerrit.wikimedia.org/r/343294

Change 343288 merged by jenkins-bot:
[apps/android/wikipedia] Hygiene: rename ImageUrlMap to ImageUrlHtmlParser

https://gerrit.wikimedia.org/r/343288

Change 343291 merged by jenkins-bot:
[apps/android/wikipedia] Hygiene: add jsoup v1.10.2

https://gerrit.wikimedia.org/r/343291

Change 343292 merged by jenkins-bot:
[apps/android/wikipedia@master] Update: replace underlying image URL parser

https://gerrit.wikimedia.org/r/343292

Change 340907 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia@master] Update: remove file-based saved pages

https://gerrit.wikimedia.org/r/340907

Change 344261 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia@master] Chore: add PageClient tests

https://gerrit.wikimedia.org/r/344261

Change 344262 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[apps/android/wikipedia@master] Chore: remove pageCombo endpoint

https://gerrit.wikimedia.org/r/344262

Change 343293 abandoned by Niedzielski:
Update: expose WikiSite URI

Reason:
Replaced

https://gerrit.wikimedia.org/r/343293

Change 343296 abandoned by Niedzielski:
Fix: save page downloading

Reason:
Replaced. @Mholloway, I will transplant your comment

https://gerrit.wikimedia.org/r/343296

Change 343287 abandoned by Niedzielski:
Hygiene: remove unused SavedPage methods and infer nullity

Reason:
Replaced

https://gerrit.wikimedia.org/r/343287

Change 343289 abandoned by Niedzielski:
Update: remove saved page image downloading and link remapping

Reason:
Replaced

https://gerrit.wikimedia.org/r/343289

Change 341386 abandoned by Niedzielski:
Update: remove page combo endpoint usage

Reason:
Replaced

https://gerrit.wikimedia.org/r/341386

@Dbrant @Mholloway, ok this guy is finally ready! Well, there was one tiny URI improvement from Michael I had to transplant over from a previous patchset but that can be captured in a new ticket if you want to merge them sooner rather than later (sorry, I gotta pack for an early morning flight!). I've added a buttload of tests so hopefully things are working right but please also test on your own devices. I don't recommend this change for the release but I'll leave it to you both to decide. \o/

Change 344261 merged by jenkins-bot:
[apps/android/wikipedia@master] Chore: add PageClient tests

https://gerrit.wikimedia.org/r/344261

Change 340906 merged by jenkins-bot:
[apps/android/wikipedia@master] Update: add second OkHttp cache for saved content

https://gerrit.wikimedia.org/r/340906

Change 340907 merged by jenkins-bot:
[apps/android/wikipedia@master] Update: remove file-based saved pages

https://gerrit.wikimedia.org/r/340907

Change 344262 merged by jenkins-bot:
[apps/android/wikipedia@master] Chore: remove pageCombo endpoint

https://gerrit.wikimedia.org/r/344262

Change 341387 merged by jenkins-bot:
[apps/android/wikipedia@master] Update: add cache option to PageClient

https://gerrit.wikimedia.org/r/341387

Change 343290 merged by jenkins-bot:
[apps/android/wikipedia@master] Update: use the new save page download api

https://gerrit.wikimedia.org/r/343290

ABorbaWMF added a subscriber: ABorbaWMF.

Tested on the App Build 2.5.194-alpha-2017-04-19
Devices - Nexus 4 Android 5.1 & Pixel Android 7.1.1

Added reading lists and built up some history. Tried some offline testing. Cleared the app cache and did more testing. Everything appears to be working.

Niedzielski closed this task as Resolved.Apr 24 2017, 7:59 PM