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

Fix sync resend #2959

Merged
merged 3 commits into from
Jul 22, 2019
Merged

Fix sync resend #2959

merged 3 commits into from
Jul 22, 2019

Conversation

AlexeyBarabash
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash commented Jul 19, 2019

Submitter Checklist:

Test Plan:

STR are in brave/brave-browser#5300

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@AlexeyBarabash AlexeyBarabash added the CI/skip Do not run CI builds (except noplatform) label Jul 19, 2019
@AlexeyBarabash AlexeyBarabash added bug feature/sync and removed CI/skip Do not run CI builds (except noplatform) labels Jul 19, 2019
@AlexeyBarabash AlexeyBarabash self-assigned this Jul 19, 2019
@AlexeyBarabash AlexeyBarabash marked this pull request as ready for review July 19, 2019 18:39
@AlexeyBarabash AlexeyBarabash changed the title Exp resend Fix sync resend Jul 19, 2019
@@ -90,7 +90,7 @@ BraveSyncServiceImpl::BraveSyncServiceImpl(Profile* profile) :
sync_client_.get(),
sync_prefs_.get())),
timer_(std::make_unique<base::RepeatingTimer>()),
unsynced_send_interval_(base::TimeDelta::FromMinutes(10)) {
unsynced_send_interval_(base::TimeDelta::FromMinutes(60)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to change it from 10 to 60 mins? The bug is in where we mark sync_timestamp.
If we bump it to 60 mins, consider there is an update for a certain bookmark, let's say url changes, it will take up to 60 mins for it to send the record.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unsynced_send_interval_ works here https://github.com/brave/brave-core/blob/master/components/brave_sync/client/bookmark_change_processor.cc#L943

      std::string last_send_time;
      node->GetMetaInfo("last_send_time", &last_send_time);
      if (!last_send_time.empty() &&
          // don't send more often than unsynced_send_interval_
          (base::Time::Now() -
              base::Time::FromJsTime(std::stod(last_send_time))) <
          unsynced_send_interval)
        continue;

and affects only on re-sends.

For the regular bookmark modification we will clear "last_send_time" at https://github.com/brave/brave-core/blob/master/components/brave_sync/client/bookmark_change_processor.cc#L373

void BookmarkChangeProcessor::BookmarkNodeChanged(BookmarkModel* model,
                                                  const BookmarkNode* node) {
  // clearing the sync_timestamp will put the record back in the `Unsynced` list
  model->DeleteNodeMetaInfo(node, "sync_timestamp");
  // also clear the last send time because this is a new change
  model->DeleteNodeMetaInfo(node, "last_send_time");

  model->SetNodeMetaInfo(node,
      "last_updated_time",
      std::to_string(base::Time::Now().ToJsTime()));
}

and it will not check for unsynced_send_interval at https://github.com/brave/brave-core/blob/master/components/brave_sync/client/bookmark_change_processor.cc#L939 .

So the regular send will happened once in 1 min.
And the resend will happened once in 60 min.

@AlexeyBarabash AlexeyBarabash merged commit 90a1a1d into master Jul 22, 2019
@AlexeyBarabash AlexeyBarabash deleted the exp_resend branch July 22, 2019 08:02
brave-builds pushed a commit that referenced this pull request Jul 22, 2019
brave-builds pushed a commit that referenced this pull request Jul 22, 2019
brave-builds pushed a commit that referenced this pull request Jul 22, 2019
@kjozwiak kjozwiak added this to the 0.69.x - Nightly milestone Jul 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants