Skip to content
This repository has been archived by the owner. It is now read-only.

Sync: Add fetch delay to mitigate S3 inconsistency #10052

Merged
merged 1 commit into from Jul 20, 2017
Merged

Conversation

@ayumi
Copy link
Contributor

ayumi commented Jul 19, 2017

Related to brave/sync#139

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header
Related to brave/sync#139
@ayumi ayumi requested review from diracdeltas and bsclifton Jul 19, 2017
@ayumi ayumi added the feature/sync label Jul 19, 2017
@bsclifton
Copy link
Member

bsclifton commented Jul 19, 2017

I'll have to dig into this some more... but in browser => browser testing, I'm seeing bookmarks not sync when both are open (some records are sent, but it appears to stop. No errors). However, after I quit/relaunch, it syncs up just fine.

My steps:

  1. (pyramid1) npm start with a session that has a decent # bookmarks
  2. (pyramid2) npm run start2 with a different session (has leftover bits from last sync, before I reset it)
  3. enable sync on pyramid1
  4. use keywords and join pyramid 1's group on pyramid2
  5. Add some folders + bookmarks on pyramid1
  6. it doesn't get synced
  7. exit both browser instances
  8. relaunch (step 1 + 2), it syncs up
@@ -349,7 +349,8 @@ module.exports.onSyncReady = (isFirstRun, e) => {
}
}
e.sender.send(syncMessages.FETCH_SYNC_RECORDS, categoryNames, startAt)
startAt = syncUtil.now()
// Reduce syncUtil.now() by this amount to compensate for records pending S3 consistency. See brave/sync #139
startAt = syncUtil.now() - config.fetchOffset

This comment has been minimized.

Copy link
@bsclifton

bsclifton Jul 19, 2017

Member

is this correct (the subtraction)? should it be + config.offset?

This comment has been minimized.

Copy link
@ayumi

ayumi Jul 19, 2017

Author Contributor

this is correct– this is "Find records whose timestamps are after 30s ago", looking in the past because records created 0–30s ago could be inconsistent.

This comment has been minimized.

Copy link
@bridiver

bridiver Jul 26, 2017

Collaborator

shouldn't you get the new startAt time (in a tmp var) before sending the FETCH_SYNC_RECORDS request? It's extremely unlikely now that you've added the offset, but in a resource starved machine there is still a potential for a gap between the sync end/next start times

This comment has been minimized.

Copy link
@SergeyZhukovsky

SergeyZhukovsky Jul 26, 2017

Member

it is a good point, we do it before on Android

@bsclifton bsclifton added this to the 0.18.x (Release Channel) milestone Jul 19, 2017
@ayumi
Copy link
Contributor Author

ayumi commented Jul 19, 2017

@bsclifton that is weird– when the sync is failing, do you get anything in the sync extension console log (go to chrome-extension://cjnmeadmgmiihncdidmfiabhenbggfjm/_generated_background_page.html and open dev tools)?

(I tried your test plan twice and it seemed ok for me.)

@ayumi ayumi merged commit 6d54868 into master Jul 20, 2017
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@ayumi ayumi deleted the fix/sync-consistency branch Jul 20, 2017
ayumi added a commit that referenced this pull request Jul 20, 2017
Sync: Add fetch delay to mitigate S3 inconsistency
ayumi added a commit that referenced this pull request Jul 20, 2017
Sync: Add fetch delay to mitigate S3 inconsistency
@LaurenWags
Copy link

LaurenWags commented Jul 20, 2017

Post-Sync import Laptop-Laptop scenario (complex case) - this scenario brave/sync#146 (comment) with different pyramids:
Pyramid 0 was Linux/Ubuntu VM (without the PR)
Pyramid 1 was MacOS laptop (with the PR)

No bookmarks/folders were lost and both pyramids were open during sync. Approx 6k bookmarks were synced.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.