Skip to content
This repository has been archived by the owner on Nov 1, 2023. It is now read-only.

move to Instant.elapsed() instead of time::Sleep timer #833

Merged
merged 3 commits into from
Apr 26, 2021

Conversation

bmc-msft
Copy link
Contributor

In current tokio, time::sleep().elapsed does not update unless the Sleep
is polled. as such, the execute_pending_commands never fires. This
replaces the sleep().elapsed with Instant.elapsed().

demoray and others added 2 commits April 26, 2021 11:22
In current tokio, time::sleep().elapsed does not update unless the Sleep
is polled.  as such, the execute_pending_commands never fires.  This
replaces the sleep().elapsed with Instant.elapsed().
@bmc-msft bmc-msft mentioned this pull request Apr 26, 2021
@@ -68,9 +70,9 @@ impl Agent {

loop {
self.heartbeat.alive();
if delay.is_elapsed() {
if instant.elapsed() >= PENDING_COMMANDS_DELAY {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should update this to select! in a loop, against two Interval futures that explicitly control how often we attempt to execute pending commands or update the state machine. Something like:

let cmd_interval = time::interval(PENDING_COMMANDS_INTERVAL);
let update_interval = time::interval(UPDATE_INTERVAL);

loop {
    self.heartbeat.alive();

    tokio::select! {
        _ = cmd_interval.tick() => {
            self.execute_pending_commands().await?;            
        }
        _ = update_interval.tick() => {
            if self.update().await? {
                debug!("agent done, exiting loop");
                break;                                
            }
        }
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

We'd probably want to update the busy() state impl too, so it checks if an Interval has ticked before polling the remote queue.

Copy link
Member

Choose a reason for hiding this comment

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

Also, we'd want to replace this sleep() with an Interval. The logic we'd want for the ready state is: "every UPDATE_INTERVAL, give the inner state handler an opportunity to check for available work. The state handler will then consult its own timer, so there is at least WORK_POLL_INTERVAL seconds between calls to the remote work queue."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's underlying issue is made complex based on how the state machine is implemented.

  • Free needs to sleep because it's response is dependent on the server.
  • Busy shouldn't sleep, but instead await on the output of the workers.
  • All of the other states should proceed immediately without delays.

In my opinion, the state machine's delay logic belongs in each of the individual states and instead of a loop where we manage the transitions, I think we should move to a model akin to the A Fist Full of States from Deis Labs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either way, that's a large amount of work and should be a follow-on PR.

@bmc-msft bmc-msft merged commit fde43a3 into microsoft:main Apr 26, 2021
@bmc-msft bmc-msft deleted the move-to-timer-elapsed branch April 26, 2021 18:39
@ghost ghost locked as resolved and limited conversation to collaborators May 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants