Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2942373002:
Extract AD policy scheduler into separate class (Closed)
|
Created:
3 years, 6 months ago by Thiemo Nagel Modified:
3 years, 6 months ago Reviewers:
pmarko CC:
chromium-reviews, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtract AD policy scheduler into separate class
The new code should be easier to test, easier to extend and easier to
re-use.
BUG=734776
TEST=added unittests for scheduler
Review-Url: https://codereview.chromium.org/2942373002
Cr-Commit-Position: refs/heads/master@{#481237}
Committed: https://chromium.googlesource.com/chromium/src/+/dd04f963fac90b8d3502d8e96340b07be463b760
Patch Set 1 #
Total comments: 21
Patch Set 2 : Various improvements #Patch Set 3 : Fix comment #Patch Set 4 : Simplify public interface (and implementation) #
Messages
Total messages: 36 (24 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
tnagel@chromium.org changed reviewers: + pmarko@chromium.org
Hey Pavol, could you maybe take a look at this CL? (There are still some compilation issues, thus maybe omit the GN file?) If for any reason you'd prefer somebody else doing the review, please let me know. Thank you and kind regards, Thiemo
Hi Thiemo, happy to do the review! A general question: Do we plan on keeping both PolicyScheduler and CloudPolicyRefreshScheduler around and having both as completely independent classes for the future? Re: Compilation problems: Looks like the unittest should also be chromeos-specific? Other than that, I just have a few suggestions/questions for your consideration. https://codereview.chromium.org/2942373002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/active_directory_policy_manager.h (right): https://codereview.chromium.org/2942373002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.h:61: void DoRefresh(PolicyScheduler::TaskCallback callback); Do you think it'd be worth mentioning in a comment that this is going to be executed by the scheduler? https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... File components/policy/core/common/policy_scheduler.cc (right): https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... components/policy/core/common/policy_scheduler.cc:25: void PolicyScheduler::ScheduleTask(base::TimeDelta delay) { Have you considered using SequenceChecker to make sure that member functions are not called concurrently from different threads (especially ScheduleTask vs. OnTaskDone)? https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... components/policy/core/common/policy_scheduler.cc:41: } else if (last_task_ == base::TimeTicks()) { Can this happen? ScheduleNextTask() is private and only called from OnTaskDone(), which sets last_task_ before that. https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... File components/policy/core/common/policy_scheduler.h (right): https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... components/policy/core/common/policy_scheduler.h:43: PolicyScheduler(Task task, Although it means repeating the comment on the class, consider mentioning that the constructor automatically schedules a task to run ASAP (so the caller don't think they should still call ScheduleTask() and also it might be surprising to some that |interval| is not used for the first task). https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... components/policy/core/common/policy_scheduler.h:51: void ScheduleTask(base::TimeDelta delay); Do we need the delay parameter in the public API? I.e., is it a usecase to plan a refresh after a specified delay from outside? If not, we could call this ScheduleTaskNow and drop the parameter. If this is only the testing, I think the test calling ScheduleTask(TimeDelta::Max()) could use the regular SchuedleNextTask-logic with interval==TimeDelta::Max to achieve the same behavior. Also worth mentioning in a comment what happens if a task is currently running (i.e. backoff logic also applies here). https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... components/policy/core/common/policy_scheduler.h:76: base::TimeTicks last_task_; Please add a comment that this captures the _end_ time of the last task. https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... File components/policy/core/common/policy_scheduler_unittest.cc (right): https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... components/policy/core/common/policy_scheduler_unittest.cc:59: base::WeakPtrFactory<PolicySchedulerTest> weak_ptr_factory_{this}; Technically, WeakPtr is not necessary here because the test fixture instance will always outlive the last task invocation in the tests, correct? https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... components/policy/core/common/policy_scheduler_unittest.cc:68: base::TimeDelta::Max(), base::TimeDelta()); Please consider adding /* interval */ and /* backoff_interval */ to these ctor argument lists in the tests so the reader knows which is which without having to look it up.
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Extract AD policy scheduler into separate class The new code should be easier to test, easier to extend and easier to re-use. BUG=732801 TEST=added unittests for scheduler ========== to ========== Extract AD policy scheduler into separate class The new code should be easier to test, easier to extend and easier to re-use. BUG=734776 TEST=added unittests for scheduler ==========
PTAL. > happy to do the review! Many thanks! > A general question: Do we plan on keeping both PolicyScheduler and > CloudPolicyRefreshScheduler around and having both as completely independent > classes for the future? Long-term, I'd like to merge the two. Filed issue 734776 explaining that. https://codereview.chromium.org/2942373002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/active_directory_policy_manager.h (right): https://codereview.chromium.org/2942373002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.h:61: void DoRefresh(PolicyScheduler::TaskCallback callback); On 2017/06/19 10:14:37, pmarko wrote: > Do you think it'd be worth mentioning in a comment that this is going to be > executed by the scheduler? I'm leaning towards not mentioning this because comments like this tend to get out of sync quickly and also with codesearch we have a quick and easy way to find the caller. https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... File components/policy/core/common/policy_scheduler.cc (right): https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... components/policy/core/common/policy_scheduler.cc:25: void PolicyScheduler::ScheduleTask(base::TimeDelta delay) { On 2017/06/19 10:14:37, pmarko wrote: > Have you considered using SequenceChecker to make sure that member functions are > not called concurrently from different threads (especially ScheduleTask vs. > OnTaskDone)? Good point. I've added it to all methods to save myself and the readers from having to reason whether a specific method could be called from outside the class or not. https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... components/policy/core/common/policy_scheduler.cc:41: } else if (last_task_ == base::TimeTicks()) { On 2017/06/19 10:14:37, pmarko wrote: > Can this happen? ScheduleNextTask() is private and only called from > OnTaskDone(), which sets last_task_ before that. Good catch! I think that part only made sense in the previous version of the scheduler that contained an initial delay. https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... File components/policy/core/common/policy_scheduler.h (right): https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... components/policy/core/common/policy_scheduler.h:43: PolicyScheduler(Task task, On 2017/06/19 10:14:37, pmarko wrote: > Although it means repeating the comment on the class, consider mentioning that > the constructor automatically schedules a task to run ASAP (so the caller don't > think they should still call ScheduleTask() and also it might be surprising to > some that |interval| is not used for the first task). Done. https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... components/policy/core/common/policy_scheduler.h:51: void ScheduleTask(base::TimeDelta delay); On 2017/06/19 10:14:37, pmarko wrote: > Do we need the delay parameter in the public API? I.e., is it a usecase to plan > a refresh after a specified delay from outside? If not, we could call this > ScheduleTaskNow and drop the parameter. Good point. Right now, I'm not aware of a use case other than testing. > If this is only the testing, I think the test calling > ScheduleTask(TimeDelta::Max()) could use the regular SchuedleNextTask-logic with > interval==TimeDelta::Max to achieve the same behavior. Imho it's cleaner if tests adhere to the public interface. Thus I'd tend towards keeping it as it is (least bad solution), but maybe you have another idea? > Also worth mentioning in a comment what happens if a task is currently running > (i.e. backoff logic also applies here). Actually, it doesn't. I've clarified this in the comment. https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... components/policy/core/common/policy_scheduler.h:76: base::TimeTicks last_task_; On 2017/06/19 10:14:37, pmarko wrote: > Please add a comment that this captures the _end_ time of the last task. Done. https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... File components/policy/core/common/policy_scheduler_unittest.cc (right): https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... components/policy/core/common/policy_scheduler_unittest.cc:59: base::WeakPtrFactory<PolicySchedulerTest> weak_ptr_factory_{this}; On 2017/06/19 10:14:37, pmarko wrote: > Technically, WeakPtr is not necessary here because the test fixture instance > will always outlive the last task invocation in the tests, correct? You're right. Changed to base::Unretained(). https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... components/policy/core/common/policy_scheduler_unittest.cc:68: base::TimeDelta::Max(), base::TimeDelta()); On 2017/06/19 10:14:37, pmarko wrote: > Please consider adding /* interval */ and /* backoff_interval */ to these ctor > argument lists in the tests so the reader knows which is which without having to > look it up. Good point. Done.
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... File components/policy/core/common/policy_scheduler.h (right): https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... components/policy/core/common/policy_scheduler.h:51: void ScheduleTask(base::TimeDelta delay); > Actually, it doesn't. I've clarified this in the comment. Nah, you're right, it does. And now I'm thinking, whether I should add a second "backoff" parameter to ScheduleTask(). The only use case so far is when a user clicks the "update policy" button. Ideally we'd like to have 0 timeout and 0 backoff for that whereas backoff for the scheduled policy fetch (90 minutes interval) could seem reasonable at maybe 10 minutes. (Though I hope it is never hit in practice.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for the delay! https://codereview.chromium.org/2942373002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/active_directory_policy_manager.h (right): https://codereview.chromium.org/2942373002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/active_directory_policy_manager.h:61: void DoRefresh(PolicyScheduler::TaskCallback callback); On 2017/06/19 22:29:51, Thiemo Nagel wrote: > On 2017/06/19 10:14:37, pmarko wrote: > > Do you think it'd be worth mentioning in a comment that this is going to be > > executed by the scheduler? > > I'm leaning towards not mentioning this because comments like this tend to get > out of sync quickly and also with codesearch we have a quick and easy way to > find the caller. Ack, I was just asking about this for consistency with the OnPolicyRefreshed comment. https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... File components/policy/core/common/policy_scheduler.h (right): https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... components/policy/core/common/policy_scheduler.h:51: void ScheduleTask(base::TimeDelta delay); On 2017/06/19 22:50:04, Thiemo Nagel wrote: > > Actually, it doesn't. I've clarified this in the comment. > > Nah, you're right, it does. > > And now I'm thinking, whether I should add a second "backoff" parameter to > ScheduleTask(). The only use case so far is when a user clicks the "update > policy" button. Ideally we'd like to have 0 timeout and 0 backoff for that > whereas backoff for the scheduled policy fetch (90 minutes interval) could seem > reasonable at maybe 10 minutes. (Though I hope it is never hit in practice.) Re: Omitting the parameter completely vs leaving it in for testing: I think the Reschedule test would be fine without calling this. It passes TimeDelta::Max() as interval to the ctor, so after the first task is executed, the second task should be automatically scheduled at TimeDelta::Max() delay. I believe we can just skip that call in the test. Re: Second back-off parameter: Hrm, simply setting backoff=0 would mean wasting CPU cycles unnecessarily though, wouldn't it? (while the not-yet-finished task is still running) I'm not sure about the purpose of backoff-process in general as it is used now - currently 1s gets passed in, which would functionally be almost equivalent to starting the next task immediately after the previous one finished if it took longer than interval. How do we want this to behave?
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks a lot! Could you please take another look? https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... File components/policy/core/common/policy_scheduler.h (right): https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... components/policy/core/common/policy_scheduler.h:51: void ScheduleTask(base::TimeDelta delay); > Re: Omitting the parameter completely vs leaving it in for testing: > I think the Reschedule test would be fine without calling this. It passes > TimeDelta::Max() as interval to the ctor, so after the first task is executed, > the second task should be automatically scheduled at TimeDelta::Max() delay. I > believe we can just skip that call in the test. Good point. Done. > Re: Second back-off parameter: > Hrm, simply setting backoff=0 would mean wasting CPU cycles unnecessarily > though, wouldn't it? (while the not-yet-finished task is still running) It would cause tasks to run back-to-back in case they overlap. But maybe that's actually the best thing to happen -- especially in case the "refresh policy" button is pressed by the user, the refresh should happen asap. > I'm not sure about the purpose of backoff-process in general as it is used now - > currently 1s gets passed in, which would functionally be almost equivalent to > starting the next task immediately after the previous one finished if it took > longer than interval. How do we want this to behave? Now that you ask this: I think the intention was to prevent very tight loops, but I don't think this can happen realistically. It seems to make more sense to remove it.
LGTM Thanks for your patience :) https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... File components/policy/core/common/policy_scheduler.h (right): https://codereview.chromium.org/2942373002/diff/60001/components/policy/core/... components/policy/core/common/policy_scheduler.h:51: void ScheduleTask(base::TimeDelta delay); On 2017/06/21 07:05:31, Thiemo Nagel wrote: > > Re: Omitting the parameter completely vs leaving it in for testing: > > I think the Reschedule test would be fine without calling this. It passes > > TimeDelta::Max() as interval to the ctor, so after the first task is executed, > > the second task should be automatically scheduled at TimeDelta::Max() delay. I > > believe we can just skip that call in the test. > > Good point. Done. > > > Re: Second back-off parameter: > > Hrm, simply setting backoff=0 would mean wasting CPU cycles unnecessarily > > though, wouldn't it? (while the not-yet-finished task is still running) > > It would cause tasks to run back-to-back in case they overlap. But maybe that's > actually the best thing to happen -- especially in case the "refresh policy" > button is pressed by the user, the refresh should happen asap. > > > I'm not sure about the purpose of backoff-process in general as it is used now > - > > currently 1s gets passed in, which would functionally be almost equivalent to > > starting the next task immediately after the previous one finished if it took > > longer than interval. How do we want this to behave? > > Now that you ask this: I think the intention was to prevent very tight loops, > but I don't think this can happen realistically. It seems to make more sense to > remove it. Yeah, agree that it shouldn't be make a big difference if it's 1s or 0.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/06/21 08:04:54, pmarko wrote: > LGTM > > Thanks for your patience :) Many thanks for the diligent review!
The CQ bit was checked by tnagel@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by tnagel@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1498061807871090,
"parent_rev": "3dfa47ef98c9dd80d120de8f8cfea303c8169ac4", "commit_rev":
"dd04f963fac90b8d3502d8e96340b07be463b760"}
Message was sent while issue was closed.
Description was changed from ========== Extract AD policy scheduler into separate class The new code should be easier to test, easier to extend and easier to re-use. BUG=734776 TEST=added unittests for scheduler ========== to ========== Extract AD policy scheduler into separate class The new code should be easier to test, easier to extend and easier to re-use. BUG=734776 TEST=added unittests for scheduler Review-Url: https://codereview.chromium.org/2942373002 Cr-Commit-Position: refs/heads/master@{#481237} Committed: https://chromium.googlesource.com/chromium/src/+/dd04f963fac90b8d3502d8e96340... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://chromium.googlesource.com/chromium/src/+/dd04f963fac90b8d3502d8e96340... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
