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

Mahmoud Rashad (Gerrit)

unread,

Jun 29, 2022, 9:18:16 AM6/29/22

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:
- Patch Set #11:
  Thanks for your comments! PTAL.
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.

Maksim Moskvitin (Gerrit)

unread,

Jun 29, 2022, 9:45:45 AM6/29/22

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:
- Patch Set #8, Line 39: last_refreshed_time_

- 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

- Patch Set #8, Line 39: elapsed_time

- 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.

Mahmoud Rashad (Gerrit)

unread,

Jun 29, 2022, 10:25:19 AM6/29/22

to Tricium, Maksim Moskvitin, chromium...@chromium.org

Attention is currently required from: Maksim Moskvitin.

View Change

3 comments:

Patchset:
- Patch Set #12:
  Thanks!
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

- Patch Set #8, Line 39: last_refreshed_time_

- > 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.

Maksim Moskvitin (Gerrit)

unread,

Jun 29, 2022, 10:41:42 AM6/29/22

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.

Mahmoud Rashad (Gerrit)

unread,

Jun 29, 2022, 11:11:55 AM6/29/22

to Tricium, Maksim Moskvitin, chromium...@chromium.org

Attention is currently required from: Maksim Moskvitin.

View Change

2 comments:

Patchset:
- Patch Set #13:
  WDYT?
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.

Maksim Moskvitin (Gerrit)

unread,

Jun 29, 2022, 12:06:34 PM6/29/22

to Mahmoud Rashad, Tricium, chromium...@chromium.org

Attention is currently required from: Mahmoud Rashad.

View Change

5 comments:

Patchset:
- Patch Set #13:
  Thanks! Spot a bug
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.

Mahmoud Rashad (Gerrit)

unread,

Jun 30, 2022, 1:28:29 PM6/30/22

to Tricium, Maksim Moskvitin, chromium...@chromium.org

Attention is currently required from: Maksim Moskvitin.

View Change

5 comments:

Patchset:
- Patch Set #15:
  Thanks! PTAL.
File components/sync/trusted_vault/degraded_recoverability_scheduler.cc:

- Patch Set #13, Line 42: is_null
  is_null() (this is not going to compile)

- Done

- Patch Set #13, Line 43: Min

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

- Patch Set #13, Line 49: Min

- And this is more severe here. […]
  Sorry about misusing TimeDelta::Min(), somehow I thought this is something similar to zero.

- Patch Set #13, Line 57: remaining_time

- 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.

Maksim Moskvitin (Gerrit)

unread,

Jul 1, 2022, 5:12:49 AM7/1/22

to Mahmoud Rashad, Chromium LUCI CQ, Tricium, chromium...@chromium.org

Attention is currently required from: Mahmoud Rashad.

View Change

7 comments:

Patchset:
- Patch Set #15:
  Thanks! A few more minor comments:
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:
- Patch Set #15, Line 19: current_refresh_period_
  Nit: prefer initializing in member initializer list.
- Patch Set #15, Line 47: ComputeNextRefreshRemainingTime
  Optional: maybe ComputeTimeUntilNextRefresh()?
- Patch Set #15, Line 51: elapsed_time
  const
- Patch Set #15, Line 52: remaining_time
  return directly, no need for local variable.

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

Mahmoud Rashad (Gerrit)

unread,

Jul 1, 2022, 6:45:40 AM7/1/22

to Chromium LUCI CQ, Tricium, Maksim Moskvitin, chromium...@chromium.org

Attention is currently required from: Maksim Moskvitin.

View Change

7 comments:

Patchset:
- Patch Set #16:
  Thanks! PTAL.
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"?

- 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 o […]
  Done
File components/sync/trusted_vault/degraded_recoverability_scheduler.cc:

- Patch Set #15, Line 19: current_refresh_period_
  Nit: prefer initializing in member initializer list.

- Done

- Patch Set #15, Line 47: ComputeNextRefreshRemainingTime
  Optional: maybe ComputeTimeUntilNextRefresh()?

- Done

- Patch Set #15, Line 51: elapsed_time
  const

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

- Patch Set #15, Line 52: remaining_time
  return directly, no need for local variable.

- Done

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

Maksim Moskvitin (Gerrit)

unread,

Jul 1, 2022, 7:46:46 AM7/1/22

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:

- Patch Set #15, Line 29: ComputeNextRefreshRemainingTime

- "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:
- Patch Set #15, Line 51: elapsed_time

- 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:
- Patch Set #16, Line 16: current_
  nit: can drop "current" from the name, it's not really relevant for this function.

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

Mahmoud Rashad (Gerrit)

unread,

Jul 1, 2022, 9:08:35 AM7/1/22

to Maksim Moskvitin, Chromium LUCI CQ, Tricium, chromium...@chromium.org

View Change

4 comments:

Patchset:
- Patch Set #17:
  Thanks!
File components/sync/trusted_vault/degraded_recoverability_scheduler.h:

- Patch Set #15, Line 29: ComputeNextRefreshRemainingTime

- > "or at least make const"? […]
  Got it.
File components/sync/trusted_vault/degraded_recoverability_scheduler.cc:

- Patch Set #15, Line 51: elapsed_time

- 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:

- Patch Set #16, Line 16: current_
  nit: can drop "current" from the name, it's not really relevant for this function.

- Done

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

Mahmoud Rashad (Gerrit)

unread,

Jul 1, 2022, 9:08:42 AM7/1/22

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.

Mahmoud Rashad (Gerrit)

unread,

Jul 2, 2022, 4:13:57 AM7/2/22

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.

Chromium LUCI CQ (Gerrit)

unread,

Jul 2, 2022, 4:17:34 AM7/2/22

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.

Reply all

Reply to author

Forward