Incremental sync follow up: transactionally merge buffers

NEW
Unassigned

Status

()

P5
normal
2 years ago
22 days ago

People

(Reporter: Grisha, Unassigned, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

2 years ago
Merging of a buffer should be done transactionally, whenever possible. One way to facilitate this is via the buffering session middleware: add signaling to let the wrapped session know that we're about to start merging, and that we're done.
Priority: -- → P2
(Reporter)

Comment 1

2 years ago
Some notes on implementing this.

The idea here is that we know we are about to start merging in a buffer full of records, and we'd like to do so transactionally.

To expand on what's described in Comment 0, perhaps one crude approach is to create/end transactions via BrowserProvider's call interface from our wrapped RepositorySessions (which are told to do so by BufferingMiddlewareRepositorySession [0]). We must really pay attention here to ensure transactions aren't left open in case of problems, are committed properly, etc.

Perhaps a better approach is to accept that this kind of buffering is here to stay, and to introduce a new RepositorySession `storeRecords` interface. Instead of looping at [0], we can hand over records to the inner repository and assume it'll "do the right thing". Local sessions tend to perform record reconciliation in their `store` methods, and build up queues of records to flush. If we want to avoid refactoring much of this flow in this bug, one incremental improvement would be to ensure that these queues are large enough (or likely unbounded) so that they're able to hold all of the changes that might be in a buffer, and to flush them in a way that's possibly very similar to how history data accessor transactionally flushes queues of records via BrowserProvider's call interface (see [1] and [2]). Special attention must be paid to bookmarks, as it has separate queues for insertions and deletions of folders and non-folders.

A tractable approach might be to land these changes separately for different repositories, or perhaps tackle changes for bookmarks separately from the rest of collections, as they're inherently a bit more complicated.

Whatever approach to this is attempted, it must at least consider how user's interactions with existing data during the buffer merge process data might affect things.

[0] https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/BufferingMiddlewareRepositorySession.java?q=path%3ABufferingMiddlewareRepositorySession.java&redirect_type=single#131-139
[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryDataAccessor.java?q=path%3AHistoryDataAccessor&redirect_type=single#133-139
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java#2245
(Reporter)

Updated

2 years ago
Mentor: gkruglov

Updated

10 months ago
Product: Android Background Services → Firefox for Android
Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.
Priority: P2 → P5
You need to log in before you can comment on or make changes to this bug.