Skip to content

Simple task scheduler#1861

Merged
mcculls merged 9 commits intomasterfrom
mcculls/simpleTaskScheduler
Sep 23, 2020
Merged

Simple task scheduler#1861
mcculls merged 9 commits intomasterfrom
mcculls/simpleTaskScheduler

Conversation

@mcculls
Copy link
Contributor

@mcculls mcculls commented Sep 14, 2020 •
edited
Loading

Changes:

  • Renamed CommonTaskExecutor to AgentTaskScheduler to better reflect this is the agent's own task scheduler
  • Simplified the scheduler to be a single thread that takes work off a DelayQueue
  • Added support for alternative ways to clear the task target

@richardstartin
Copy link
Contributor

richardstartin commented Sep 14, 2020

Since I have encountered initialisation order problems in the past (nice to see you've removed some duct tape introduced in #1812), I think we are better off with an inversion of control model than singletons, which will lead to deterministic initialisation order. I think this would be a nice direction for this PR to take.

bantonsson
bantonsson reviewed Sep 14, 2020
View reviewed changes
Copy link
Contributor

@bantonsson bantonsson left a comment

Choose a reason for hiding this comment

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

Great that you untangle this!

@richardstartin
Copy link
Contributor

richardstartin commented Sep 15, 2020

As discussed offline, the question of deterministic initialisation is another matter and can be addressed independently of this.

@mcculls mcculls force-pushed the mcculls/simpleTaskScheduler branch from 066b8a2 to c4e4d67 Compare September 17, 2020 11:06
@richardstartin
Copy link
Contributor

richardstartin commented Sep 18, 2020

Can we prioritise getting this merged?

richardstartin
richardstartin previously approved these changes Sep 18, 2020
View reviewed changes
@richardstartin richardstartin mentioned this pull request Sep 18, 2020
Merged
bantonsson
bantonsson previously approved these changes Sep 18, 2020
View reviewed changes
@mcculls mcculls force-pushed the mcculls/simpleTaskScheduler branch from c4e4d67 to 9b1cca9 Compare September 18, 2020 15:17
@mcculls mcculls force-pushed the mcculls/simpleTaskScheduler branch from 2b8aaa2 to 7271fc4 Compare September 23, 2020 08:31
@mcculls mcculls marked this pull request as ready for review September 23, 2020 08:31
@mcculls mcculls requested a review from a team as a code owner September 23, 2020 08:31
@mcculls mcculls dismissed stale reviews from bantonsson and richardstartin September 23, 2020 08:48

stale review from when PR was a draft

richardstartin
richardstartin approved these changes Sep 23, 2020
View reviewed changes
Copy link
Contributor

@richardstartin richardstartin left a comment

Choose a reason for hiding this comment

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

Looks really good. Not related to this PR but I think it's probably a mistake to be using the CommonTaskExecutor in DDAgentApi. We can fix that in another PR.

@mcculls
Copy link
Contributor Author

mcculls commented Sep 23, 2020

re. DDAgentApi - I replaced that use of the executor with one that always throws an exception - the intent according to the comments was to make sure we didn't accidentally trigger an async call (ie. all calls are expected to be synchronous) and also avoid starting OkHttpClient's own executor. The rejecting executor should satisfy both of those needs.

@richardstartin richardstartin mentioned this pull request Sep 23, 2020
Merged
@mcculls mcculls merged commit 1704b37 into master Sep 23, 2020
@mcculls mcculls deleted the mcculls/simpleTaskScheduler branch September 23, 2020 10:27
@github-actions github-actions bot added this to the 0.64.0 milestone Sep 23, 2020
@mcculls mcculls mentioned this pull request Dec 4, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bantonsson bantonsson bantonsson approved these changes

+1 more reviewer

@richardstartin richardstartin richardstartin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.64.0

Development

Successfully merging this pull request may close these issues.

3 participants

@mcculls @richardstartin @bantonsson