[Sync] Add the basic logic of the DegradedRecoverabilityScheduler class [chromium/src : main]

0 views
Skip to first unread message

Mahmoud Rashad (Gerrit)

unread,
Jun 29, 2022, 9:18:16 AMJun 29
to Tricium, Maksim Moskvitin, chromium...@chromium.org

Attention is currently required from: Maksim Moskvitin.

View Change

8 comments:

  • Commit Message:

    • Patch Set #8, Line 9: adds

      Can you please clarify in the CL description, that the class is not complete yet and it contains onl […]

      Done

  • Patchset:

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.h:

    • Patch Set #8, Line 33: current_refresh_period_

      Better to initialize in ctor (kLongRefreshPeriod looks like a good default).

      Done

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.cc:

    • Patch Set #5, Line 64:

        // TODO(crbug.com/1247990): Make sure the to schedule the next Refresh() after
      // the current one is completed.

      I see, then I'd suggest to either drop this comment (following "To be implemented" above) or rephras […]

      Done

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.cc:

    • Patch Set #8, Line 19: last_refreshed_time_

      nit: put variable name into `` (note: |name| is more widespread in the codebase, but backticks is a […]

      Done

    • Patch Set #8, Line 34: DCHECK(next_refresh_timer_.IsRunning());

      I'm not very comfortable with DCHECK() that clearly could be violated - even though this is going to […]

      Done

    • Patch Set #8, Line 39: elapsed_time

      You need to take into account, that it might be negative (Now() is not guaranteed to be incremental, […]

      In this case we can refresh now, do you recommend to handle this in a different way? or is there a way to listen to the system time and maintain the `last_refreshed_time_`?

    • Patch Set #8, Line 39: last_refreshed_time_

      Need to handle uninitialized value (even if we read this from disk, this could be first run)

      I was planning to make sure it's initialized in the ctor with a fake value (e.g. Time::Now() - `kLongRefreshPeriod`) if this is the first run, alternatively we can declare `last_refreshed_time_` as absl::optional<base::Time> and handle if it's not initialized yet, WDYT?

To view, visit change 3721576. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I10408bfec938cbfec87b579e568c2d920bbace5e
Gerrit-Change-Number: 3721576
Gerrit-PatchSet: 11
Gerrit-Owner: Mahmoud Rashad <mmra...@google.com>
Gerrit-Reviewer: Maksim Moskvitin <mmosk...@google.com>
Gerrit-Attention: Maksim Moskvitin <mmosk...@google.com>
Gerrit-Comment-Date: Wed, 29 Jun 2022 13:18:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Maksim Moskvitin <mmosk...@google.com>
Comment-In-Reply-To: Mahmoud Rashad <mmra...@google.com>
Gerrit-MessageType: comment

Maksim Moskvitin (Gerrit)

unread,
Jun 29, 2022, 9:45:45 AMJun 29
to Mahmoud Rashad, Tricium, chromium...@chromium.org

Attention is currently required from: Mahmoud Rashad.

View Change

2 comments:

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.cc:

    • I was planning to make sure it's initialized in the ctor with a fake value (e.g. Time::Now() - kLongRefreshPeriod ) if this is the first run

    • It might be confusing when reading the code.

      Either using `optional` or built-in nullable time (see is_null() method) SGTM

    • In this case we can refresh now, do you recommend to handle this in a different way?

      No, I don't think this is good idea. Since there are no guarantees around Now() being incremental and perhaps less rare events like "system time was adjusted automatically", leading to more often request than we want. Instead I'd suggest setting delay to `current_refresh_period`.

To view, visit change 3721576. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I10408bfec938cbfec87b579e568c2d920bbace5e
Gerrit-Change-Number: 3721576
Gerrit-PatchSet: 11
Gerrit-Owner: Mahmoud Rashad <mmra...@google.com>
Gerrit-Reviewer: Maksim Moskvitin <mmosk...@google.com>
Gerrit-Attention: Mahmoud Rashad <mmra...@google.com>
Gerrit-Comment-Date: Wed, 29 Jun 2022 13:45:33 +0000

Mahmoud Rashad (Gerrit)

unread,
Jun 29, 2022, 10:25:19 AMJun 29
to Tricium, Maksim Moskvitin, chromium...@chromium.org

Attention is currently required from: Maksim Moskvitin.

View Change

3 comments:

  • Patchset:

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.cc:

    • Patch Set #8, Line 39: elapsed_time

      > In this case we can refresh now, do you recommend to handle this in a different way? […]

      Done

    • > I was planning to make sure it's initialized in the ctor with a fake value (e.g. […]

      Done

To view, visit change 3721576. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I10408bfec938cbfec87b579e568c2d920bbace5e
Gerrit-Change-Number: 3721576
Gerrit-PatchSet: 12
Gerrit-Owner: Mahmoud Rashad <mmra...@google.com>
Gerrit-Reviewer: Maksim Moskvitin <mmosk...@google.com>
Gerrit-Attention: Maksim Moskvitin <mmosk...@google.com>
Gerrit-Comment-Date: Wed, 29 Jun 2022 14:25:06 +0000

Maksim Moskvitin (Gerrit)

unread,
Jun 29, 2022, 10:41:42 AMJun 29
to Mahmoud Rashad, Tricium, chromium...@chromium.org

Attention is currently required from: Mahmoud Rashad.

View Change

2 comments:

  • Patchset:

    • Patch Set #12:

      Thanks! I think there is still something we can do to make this cleaner:

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.cc:

    • Patch Set #12, Line 44:

          // This means that either the `next_refresh_timer_` is working with almost
      // zero delay or there is a Refresh() call in progress.
      return;

      Can we instead make this working in a way, that Start() is called in constructor? I think this would be easier to follow and it looks like otherwise we will duplicate logic in constructor and Start() once we start persisting state.

To view, visit change 3721576. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I10408bfec938cbfec87b579e568c2d920bbace5e
Gerrit-Change-Number: 3721576
Gerrit-PatchSet: 12
Gerrit-Owner: Mahmoud Rashad <mmra...@google.com>
Gerrit-Reviewer: Maksim Moskvitin <mmosk...@google.com>
Gerrit-Attention: Mahmoud Rashad <mmra...@google.com>
Gerrit-Comment-Date: Wed, 29 Jun 2022 14:41:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Mahmoud Rashad (Gerrit)

unread,
Jun 29, 2022, 11:11:55 AMJun 29
to Tricium, Maksim Moskvitin, chromium...@chromium.org

Attention is currently required from: Maksim Moskvitin.

View Change

2 comments:

  • Patchset:

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.cc:

    • Patch Set #12, Line 44:

          // This means that either the `next_refresh_timer_` is working with almost
      // zero delay or there is a Refresh() call in progress.
      return;

    • Can we instead make this working in a way, that Start() is called in constructor? I think this would […]

      Done

To view, visit change 3721576. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I10408bfec938cbfec87b579e568c2d920bbace5e
Gerrit-Change-Number: 3721576
Gerrit-PatchSet: 13
Gerrit-Owner: Mahmoud Rashad <mmra...@google.com>
Gerrit-Reviewer: Maksim Moskvitin <mmosk...@google.com>
Gerrit-Attention: Maksim Moskvitin <mmosk...@google.com>
Gerrit-Comment-Date: Wed, 29 Jun 2022 15:11:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Maksim Moskvitin <mmosk...@google.com>
Gerrit-MessageType: comment

Maksim Moskvitin (Gerrit)

unread,
Jun 29, 2022, 12:06:34 PMJun 29
to Mahmoud Rashad, Tricium, chromium...@chromium.org

Attention is currently required from: Mahmoud Rashad.

View Change

5 comments:

  • Patchset:

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.cc:

    • Patch Set #13, Line 42: is_null

      is_null() (this is not going to compile)

    • Patch Set #13, Line 43: Min

      Oh, just noticed this is actually negative. It's going to work, but seems like timer owners aren't very happy about it: https://source.chromium.org/chromium/chromium/src/+/main:base/timer/timer.cc;drc=8cfba4337a86e56ab78e1f758b2d6614a15fa346;l=198

      Just use TimeDelta()?

    • Patch Set #13, Line 49: Min

      And this is more severe here. elapsed_time is now (-inf), later integer overflow when computing remaining time.

      So, this logic is somewhat hard to get right and I start to feel more strongly about adding some tests together with the first CL. Would you be fine with that? This should become testable if you simply pass "const base::RepeatingCallback& refresh_callback" in constructor and call it in refresh. Of course I'm happy to help as needed.

    • Patch Set #13, Line 57: remaining_time

      nit: I'd factor out the logic, e.g. introduce something like ComputeRemainingTime(last_refresh, refresh_period), which would sometimes return TimeDelta() (in the cases when you pass TimeDelta::Min() right now). This way we'd avoid multiple Start() callers.

To view, visit change 3721576. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I10408bfec938cbfec87b579e568c2d920bbace5e
Gerrit-Change-Number: 3721576
Gerrit-PatchSet: 13
Gerrit-Owner: Mahmoud Rashad <mmra...@google.com>
Gerrit-Reviewer: Maksim Moskvitin <mmosk...@google.com>
Gerrit-Attention: Mahmoud Rashad <mmra...@google.com>
Gerrit-Comment-Date: Wed, 29 Jun 2022 16:06:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Mahmoud Rashad (Gerrit)

unread,
Jun 30, 2022, 1:28:29 PMJun 30
to Tricium, Maksim Moskvitin, chromium...@chromium.org

Attention is currently required from: Maksim Moskvitin.

View Change

5 comments:

  • Patchset:

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.cc:

    • Done

    • Oh, just noticed this is actually negative. […]

      Done

    • And this is more severe here. […]

      Sorry about misusing TimeDelta::Min(), somehow I thought this is something similar to zero.

    • nit: I'd factor out the logic, e.g. […]

      Done

To view, visit change 3721576. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I10408bfec938cbfec87b579e568c2d920bbace5e
Gerrit-Change-Number: 3721576
Gerrit-PatchSet: 15
Gerrit-Owner: Mahmoud Rashad <mmra...@google.com>
Gerrit-Reviewer: Maksim Moskvitin <mmosk...@google.com>
Gerrit-Attention: Maksim Moskvitin <mmosk...@google.com>
Gerrit-Comment-Date: Thu, 30 Jun 2022 17:28:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No

Maksim Moskvitin (Gerrit)

unread,
Jul 1, 2022, 5:12:49 AMJul 1
to Mahmoud Rashad, Chromium LUCI CQ, Tricium, chromium...@chromium.org

Attention is currently required from: Mahmoud Rashad.

View Change

7 comments:

  • Patchset:

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.h:

    • Patch Set #15, Line 29: ComputeNextRefreshRemainingTime

      Move out of class to unnamed namespace (just pass required state), or at least make const.

    • Patch Set #15, Line 35: last_refreshed_time_

      Would be nice to add some comments about these members in general, but at least document that this one can null and what does it mean.

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.cc:

To view, visit change 3721576. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I10408bfec938cbfec87b579e568c2d920bbace5e
Gerrit-Change-Number: 3721576
Gerrit-PatchSet: 15
Gerrit-Owner: Mahmoud Rashad <mmra...@google.com>
Gerrit-Reviewer: Mahmoud Rashad <mmra...@google.com>
Gerrit-Reviewer: Maksim Moskvitin <mmosk...@google.com>
Gerrit-Attention: Mahmoud Rashad <mmra...@google.com>
Gerrit-Comment-Date: Fri, 01 Jul 2022 09:12:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Mahmoud Rashad (Gerrit)

unread,
Jul 1, 2022, 6:45:40 AMJul 1
to Chromium LUCI CQ, Tricium, Maksim Moskvitin, chromium...@chromium.org

Attention is currently required from: Maksim Moskvitin.

View Change

7 comments:

  • Patchset:

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.h:

    • Patch Set #15, Line 29: ComputeNextRefreshRemainingTime

      Move out of class to unnamed namespace (just pass required state), or at least make const.

    • I moved it, but what do you mean by "or at least make const"?

    • Would be nice to add some comments about these members in general, but at least document that this o […]

      Done

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.cc:

    • Done

    • Done

    • Sorry I didn't get this, there's no highlighting.

    • Done

To view, visit change 3721576. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I10408bfec938cbfec87b579e568c2d920bbace5e
Gerrit-Change-Number: 3721576
Gerrit-PatchSet: 16
Gerrit-Owner: Mahmoud Rashad <mmra...@google.com>
Gerrit-Reviewer: Mahmoud Rashad <mmra...@google.com>
Gerrit-Reviewer: Maksim Moskvitin <mmosk...@google.com>
Gerrit-Attention: Maksim Moskvitin <mmosk...@google.com>
Gerrit-Comment-Date: Fri, 01 Jul 2022 10:45:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No

Maksim Moskvitin (Gerrit)

unread,
Jul 1, 2022, 7:46:46 AMJul 1
to Mahmoud Rashad, Chromium LUCI CQ, Tricium, chromium...@chromium.org

Attention is currently required from: Mahmoud Rashad.

Patch set 16:Code-Review +1

View Change

4 comments:

  • Patchset:

    • Patch Set #16:

      Thanks! LGTM % last two nits (argument name and const variable)

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.h:

    • "or at least make const"?
      Marking method as const, so it's clear that it doesn't change state of the object.

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.cc:

    • Sorry I didn't get this, there's no highlighting.

    • Variables ideally should be const if you're not going to change it (e.g. const base::TimeDelta elapsed_time = ...).

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.cc:

To view, visit change 3721576. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I10408bfec938cbfec87b579e568c2d920bbace5e
Gerrit-Change-Number: 3721576
Gerrit-PatchSet: 16
Gerrit-Owner: Mahmoud Rashad <mmra...@google.com>
Gerrit-Reviewer: Mahmoud Rashad <mmra...@google.com>
Gerrit-Reviewer: Maksim Moskvitin <mmosk...@google.com>
Gerrit-Attention: Mahmoud Rashad <mmra...@google.com>
Gerrit-Comment-Date: Fri, 01 Jul 2022 11:46:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes

Mahmoud Rashad (Gerrit)

unread,
Jul 1, 2022, 9:08:35 AMJul 1
to Maksim Moskvitin, Chromium LUCI CQ, Tricium, chromium...@chromium.org

View Change

4 comments:

  • Patchset:

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.h:

    • > "or at least make const"? […]

      Got it.

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.cc:

    • Variables ideally should be const if you're not going to change it (e.g. […]

      Done

  • File components/sync/trusted_vault/degraded_recoverability_scheduler.cc:

    • Done

To view, visit change 3721576. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I10408bfec938cbfec87b579e568c2d920bbace5e
Gerrit-Change-Number: 3721576
Gerrit-PatchSet: 17
Gerrit-Owner: Mahmoud Rashad <mmra...@google.com>
Gerrit-Reviewer: Mahmoud Rashad <mmra...@google.com>
Gerrit-Reviewer: Maksim Moskvitin <mmosk...@google.com>
Gerrit-Comment-Date: Fri, 01 Jul 2022 13:08:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No

Mahmoud Rashad (Gerrit)

unread,
Jul 1, 2022, 9:08:42 AMJul 1
to Maksim Moskvitin, Chromium LUCI CQ, Tricium, chromium...@chromium.org

Patch set 17:Commit-Queue +2

View Change

    To view, visit change 3721576. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I10408bfec938cbfec87b579e568c2d920bbace5e
    Gerrit-Change-Number: 3721576
    Gerrit-PatchSet: 17
    Gerrit-Owner: Mahmoud Rashad <mmra...@google.com>
    Gerrit-Reviewer: Mahmoud Rashad <mmra...@google.com>
    Gerrit-Reviewer: Maksim Moskvitin <mmosk...@google.com>
    Gerrit-Comment-Date: Fri, 01 Jul 2022 13:08:28 +0000
    Gerrit-HasComments: No
    Gerrit-Has-Labels: Yes
    Gerrit-MessageType: comment

    Mahmoud Rashad (Gerrit)

    unread,
    Jul 2, 2022, 4:13:57 AMJul 2
    to Maksim Moskvitin, Chromium LUCI CQ, Tricium, chromium...@chromium.org

    Attention is currently required from: Mahmoud Rashad.

    Patch set 17:Commit-Queue +2

    View Change

      To view, visit change 3721576. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I10408bfec938cbfec87b579e568c2d920bbace5e
      Gerrit-Change-Number: 3721576
      Gerrit-PatchSet: 17
      Gerrit-Owner: Mahmoud Rashad <mmra...@google.com>
      Gerrit-Reviewer: Mahmoud Rashad <mmra...@google.com>
      Gerrit-Reviewer: Maksim Moskvitin <mmosk...@google.com>
      Gerrit-Attention: Mahmoud Rashad <mmra...@google.com>
      Gerrit-Comment-Date: Sat, 02 Jul 2022 08:13:47 +0000

      Chromium LUCI CQ (Gerrit)

      unread,
      Jul 2, 2022, 4:17:34 AMJul 2
      to Mahmoud Rashad, Maksim Moskvitin, Tricium, chromium...@chromium.org

      Chromium LUCI CQ submitted this change.

      View Change



      16 is the latest approved patch-set.
      The change was submitted with unreviewed changes in the following files:

      ```
      The name of the file: components/sync/trusted_vault/degraded_recoverability_scheduler.cc
      Insertions: 4, Deletions: 3.

      @@ -13,13 +13,14 @@
      constexpr base::TimeDelta kShortRefreshPeriod = base::Hours(1);

      base::TimeDelta ComputeTimeUntilNextRefresh(
      - const base::TimeDelta& current_refresh_period,
      + const base::TimeDelta& refresh_period,
      const base::TimeTicks& last_refreshed_time) {
      if (last_refreshed_time.is_null()) {
      return base::TimeDelta();
      }
      - base::TimeDelta elapsed_time = base::TimeTicks::Now() - last_refreshed_time;
      - return current_refresh_period - elapsed_time;
      + const base::TimeDelta elapsed_time =
      + base::TimeTicks::Now() - last_refreshed_time;
      + return refresh_period - elapsed_time;
      }

      } // namespace
      ```

      Approvals: Maksim Moskvitin: Looks good to me Mahmoud Rashad: Commit
      [Sync] Add the basic logic of the DegradedRecoverabilityScheduler class

      This CL adds only the basic scheduling logic of the
      DegradedRecoverabilityScheduler class without any behaviour changes, it
      is not complete yet.

      Bug: 1247990
      Change-Id: I10408bfec938cbfec87b579e568c2d920bbace5e
      Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3721576
      Reviewed-by: Maksim Moskvitin <mmosk...@google.com>
      Commit-Queue: Mahmoud Rashad <mmra...@google.com>
      Cr-Commit-Position: refs/heads/main@{#1020355}
      ---
      M components/sync/trusted_vault/BUILD.gn
      A components/sync/trusted_vault/degraded_recoverability_scheduler.cc
      A components/sync/trusted_vault/degraded_recoverability_scheduler.h
      3 files changed, 137 insertions(+), 0 deletions(-)


      To view, visit change 3721576. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I10408bfec938cbfec87b579e568c2d920bbace5e
      Gerrit-Change-Number: 3721576
      Gerrit-PatchSet: 18
      Gerrit-Owner: Mahmoud Rashad <mmra...@google.com>
      Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
      Gerrit-Reviewer: Mahmoud Rashad <mmra...@google.com>
      Gerrit-Reviewer: Maksim Moskvitin <mmosk...@google.com>
      Gerrit-MessageType: merged
      Reply all
      Reply to author
      Forward
      0 new messages