1207468 - Prefix asyncStorage entries with userid in DataAdapters

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Sync, defect, P1)

Description

[Michiel de Jong [:michielbdejong]]

This is a follow-up to bug 1191773.

The way it uses an asyncStorage data base to map FxSync Id <-> DataStore Id takes up disk space and processing time, so maybe we can do something smarter.

Suggestion 1: Implement and use https://github.com/Kinto/kinto.js/issues/173
Suggestion 2: Store records in the DataStore by their FxSync Id (what would be the implications of that for other apps? would a data migration be needed)?
Suggestion 3: ... ?

Comment 1

[Michiel de Jong [:michielbdejong]]

Re suggestion 2: We're currently using the URL as the id in DataStore, so that's obviously not going to fly if we change that. https://github.com/weilonge/gaia/blob/seanlee/DataSync/master/Bug1191773/apps/sync/js/adapters/history.js#L114

Comment 2

[Michiel de Jong [:michielbdejong]]

Possible suggestion 3: A 'dry run' option in Kinto.js `Collection#sync`. That would allow us to inspect the SyncResults object from Kinto.js to know what to change in the DataStore (while still having access to the unchanged records). We could then apply the incoming changes one-by-one as we process them, meaning that if something goes wrong before we processed the whole SyncResults (e.g. app is brutally killed), we will still get those unprocessed changes on next sync. The ones that we did apply will show up as 'skipped' in the next sync. It's not so efficient in updating Kinto.js's lastModified timestamp, and will lead to extra bandwidth usage (which is not nice on mobile), but maybe we can iterate over this thought.

Comment 3

[Michiel de Jong [:michielbdejong]]

Suggestion 4 (variation on suggestion 3): Pass a second RemoteTransformer to Kinto.js, from which the adapter can inspect each incoming change before it gets committed.

Comment 4

[Sean Lee [:seanlee][:weilonge]]

Hey Michiel,

Thanks for your suggestions. :)
For suggestion 4, I checked RemoteTransformer example http://kintojs.readthedocs.org/en/latest/api/#listing-records .
Is it possible to get the original URL (aka. DataStoreID) for deleted: true record?

class MyRemoteTransformer extends Kinto.transformers.RemoteTransformer {
  encode(record) { <<<  Will it contain URL information here?
    return update(record, {title: record.title + "!"});
  }

  decode(record) {
    return update(record, {title: record.title.slice(0, -1)});
  }
}

Comment 5

[Sean Lee [:seanlee][:weilonge]]

Correct my above comment, I think the URL information would be at decode function rather than encode, and I suppose that URL will be in payload object:

  Delete Records from History Collection (HC): {
    "id": "_Avscjx5srFy",
    "sortindex": 100,
    "last_modified": 1441985077970,
    "payload": {
      "url": "http://mozilla.org",
      "id": "_Avscjx5srFy",
      "deleted": true
    },
    "_status": "synced"
  }

Comment 6

[Michiel de Jong [:michielbdejong]]

New insight: https://github.com/Kinto/kinto.js/issues/173#issuecomment-145007083

Seems nicer than spying through a RemoteTransformer.

Comment 7

[Michiel de Jong [:michielbdejong]]

As discussed today, we will store this mapping in localStorage, and prefixed with the email address of the logged in user. On the TV (but not on the phone), this data will be deleted when the user logs out.

Comment 9

[GitHub Autolander Bot]

Attachment: [gaia] michielbdejong:1207468-fxsyncid-datastoreid-mapping > mozilla-b2g:master

Comment 10

[Michiel de Jong [:michielbdejong]]

Comment on attachment 8676186 [details] [review]
[gaia] michielbdejong:1207468-fxsyncid-datastoreid-mapping > mozilla-b2g:master

Treeherder shows an ESL error, but I can't see why?

Comment 11

[Michiel de Jong [:michielbdejong]]

(In reply to Michiel de Jong [:michielbdejong] from comment #10)
> Comment on attachment 8676186 [details] [review]
> [gaia] michielbdejong:1207468-fxsyncid-datastoreid-mapping >
> mozilla-b2g:master
> 
> Treeherder shows an ESL error, but I can't see why?

Ah, green after a rebase.

Please review.

Comment 12

[Michiel de Jong [:michielbdejong]]

IIRC, when the Sync server is flushed, kB and xClientState will change, right? We should find out if the fxSyncIds also change in this case. If they do, then using xClientState here is correct, but if not then it's more correct to use the email address of the current user.

Comment 13

[Michiel de Jong [:michielbdejong]]

(In reply to Michiel de Jong [:michielbdejong] from comment #12)
> IIRC, when the Sync server is flushed, kB and xClientState will change,
> right? We should find out if the fxSyncIds also change in this case. If they
> do, then using xClientState here is correct, but if not then it's more
> correct to use the email address of the current user.

Created P2 follow-up bug 1218286 about this.

Comment 14

[Michiel de Jong [:michielbdejong]]

Incorporated your first review comment https://github.com/mozilla-b2g/gaia/pull/32596#discussion_r42936908 and created a follow-up bug for your second review comment https://github.com/mozilla-b2g/gaia/pull/32596#discussion_r42936922. Rebased, tests are green.

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

https://github.com/mozilla-b2g/gaia/commit/4c78c9ea90b1a89734df246ccfa374f4f0aafe5b