Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resource Cluster Aware Scheduler Introduction #182

Merged
merged 3 commits into from
Apr 19, 2022

Conversation

sundargates
Copy link
Collaborator

Context

This diff introduces a new scheduler based on resource cluster fundamentals.

Checklist

  • ./gradlew build compiles code correctly
  • Added new tests where applicable
  • ./gradlew test passes all tests
  • Extended README or added javadocs where applicable
  • Added copyright headers for new files from CONTRIBUTING.md

@github-actions
Copy link

github-actions bot commented Apr 18, 2022

Unit Test Results

109 files  +  38  109 suites  +38   5m 25s ⏱️ + 3m 25s
477 tests +109  458 ✔️ +108  19 💤 +2  0  - 1 

Results for commit 9030ffc. ± Comparison against base commit b1a3063.

♻️ This comment has been updated with latest results.

/**
* Factory for the Mantis Scheduler based on the JobDefinition
*/
public interface MantisSchedulerFactory {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we keep interface naming with prefix "I"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not too fond of that convention, honestly. I don't mind following it if it's consistently followed. However, from whatever I can see from the current Mantis code, I don't think a lot of components follow that style.

@sundargates sundargates changed the title Resource Cluster Aware Scheduler Introuction Resource Cluster Aware Scheduler Introduction Apr 18, 2022
}

@Override
public Receive createReceive() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what about when the scheduler is not master? should it stop scheduling requests etc..?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if the scheduler is not master, then the default ServiceLifecycle shuts down the master and waits for it to get restarted.

Comment on lines +116 to +120
TaskExecutorGateway gateway =
resourceCluster.getTaskExecutorGateway(event.getTaskExecutorID()).join();

TaskExecutorRegistration info =
resourceCluster.getTaskExecutorInfo(event.getTaskExecutorID()).join();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are the two 'get' calls here involve any long IO ops for blocking concern?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, these should be pretty cheap. The only reason they get async responses is because they send fire-and-forget messages to the actors involved.

@sundargates sundargates merged commit 85d9687 into Netflix:master Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants