Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Poll device list during long initial commits procedure #17077

Merged
merged 1 commit into from
Feb 10, 2023

Conversation

AlexeyBarabash
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash commented Feb 7, 2023

Issue happens because at Syncer::BuildAndPostCommits loop is executed till all the records will be sent. It can take significant amount of time, ~10 minutes for 20k records, and during this time device can't receive updates.

This PR adds forced get updates only of device type to get the updates if large commit initial happens.

Resolves brave/brave-browser#27931

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

import_20k.zip

Test plan

  1. For device1 import bookmarks from import_20k/import1 folder from attached zip. Similar for device2 => import_20k/import1;
  2. On device1 create the sync chain;
  3. Connect device2 to the sync chain, expected ti see device1 in devices list;
  4. Expected to see on device1's list the device2 in approx ~15..35 seconds
  5. Connect device3 to the sync chain, expected to see device1;device2;device3 on each device list in approx ~15..35 seconds.

@AlexeyBarabash AlexeyBarabash force-pushed the sync_force_fetch_devices branch 2 times, most recently from e33218e to 46470c3 Compare February 7, 2023 19:22
@AlexeyBarabash AlexeyBarabash changed the title WIP: poll devices during long initial commits procedure to be aware o… Poll device list during long initial commits procedure Feb 7, 2023
@AlexeyBarabash AlexeyBarabash marked this pull request as ready for review February 7, 2023 21:01
@AlexeyBarabash AlexeyBarabash requested review from a team as code owners February 7, 2023 21:01
@AlexeyBarabash AlexeyBarabash added the CI/skip Do not run CI builds (except noplatform) label Feb 8, 2023
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

@AlexeyBarabash AlexeyBarabash removed the CI/skip Do not run CI builds (except noplatform) label Feb 9, 2023
@kjozwiak
Copy link
Member

Reproduced the original issue using the STR/Cases outlined via brave/brave-browser#27931 (comment) on Win 11 x64 using the following build(s):

Brave | 1.50.25 Chromium: 110.0.5481.77 (Official Build) nightly (64-bit) 
--- | ---
Revision | 65ed616c6e8ee3fe0ad64fe83796c020644d42af-refs/branch-heads/5481@{#839}
OS | Windows 11 Version 22H2 (Build 22621.1265)

Using a Win 11 x64 (Device A) machine and a Win 10 x64 VM (Device B), I reproduced the issue mentioned via brave/brave-browser#27931 (comment) where it took almost ~10-15min for the Win 10 x64 (Device B) machine to appear in the list under brave://sync via the Win 11 x64 machine (Device A). I went through the above a few times and consistently ran into the issue.

Example of Device B not appearing in Device A after 10-15min

Example Example Example
device1sync1 device2sync1 notSyncing

Syncing about 10-15min

syncingafter10min


Verification PASSED on Win 11 x64, Win 10 x64 & Pixel 6 running Android 13 using the following build(s):

Brave | 1.50.54 Chromium: 110.0.5481.104 (Official Build) nightly (64-bit)
-- | --
Revision | 46de4a7f41979e829b430bc1ee30ef483aa227ac-refs/branch-heads/5481_77@{#19}
  • Win 11 x64 - Desktop - Device A
  • Win 10 x64 - VM - Device B
  • Pixel 8 running Android 13 - Device C

Using the same STR/Cases I ran through the above with the guidance of #17077 (comment), ensured that Device A, Device B and Device C all appeared via brave://sync within ~15s-1min rather than the previous ~10-15mins.

Example of Device A, Device B & Device C appearing in brave://sync within 1min on Desktop

goodOneSync

Example of Device A, Device B & Device C appearing in brave://sync within 1min on Android

Example Example
Screenshot_20230221-224007 Screenshot_20230221-224016

I also ensured that import1 & import2 folders were being synced between the three devices. Sometimes this took a bit of time but assuming it's because how large these bookmark files are. However, they always eventually sync even though it takes a bit of time. But the devices get added within ~1min which is a huge improvement compared to the previous ~10mins or more.

kjozwiak pushed a commit that referenced this pull request Feb 22, 2023
kjozwiak pushed a commit that referenced this pull request Feb 22, 2023
Copy link
Collaborator

@cdesouza-chromium cdesouza-chromium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

AlexeyBarabash added a commit that referenced this pull request Jan 25, 2024
AlexeyBarabash added a commit that referenced this pull request Jan 29, 2024
AlexeyBarabash added a commit that referenced this pull request Jan 29, 2024
kjozwiak pushed a commit that referenced this pull request Jan 31, 2024
… (uplift to 1.63.x) (#21807)

Uplift of #21746 (squashed) to beta
kjozwiak pushed a commit that referenced this pull request Jan 31, 2024
… (uplift to 1.62.x) (#21806)

Uplift of #21746 (squashed) to release
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

[Sync] Long wait for updates when starting sync device with big amount of objects
5 participants