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

Refactor mesos scheduler initialization #1152

Merged
merged 11 commits into from Jun 12, 2019

Conversation

Projects
None yet
3 participants
@pschorf
Copy link
Contributor

commented May 31, 2019

Changes proposed in this PR

  • Add a new method to compute cluster, initialize
  • Move mesos scheduler and driver creation from start-mesos-scheduler to initialize
  • Rename start-mesos-scheduler to start-leader-selector
  • Remove set-mesos-driver-hack!

Why are we making these changes?

This makes the initialization logic less mesos-specific, and also moves towards supporting multiple compute clusters (since you could call initialize on several clusters)

@pschorf pschorf changed the title WIP: Refactor mess scheduler initialization WIP: Refactor mesos scheduler initialization May 31, 2019

@pschorf pschorf added the wip label May 31, 2019

@pschorf pschorf force-pushed the pschorf:init-refactor branch from 2519140 to 0c6ade1 May 31, 2019

@pschorf pschorf removed the wip label May 31, 2019

@pschorf pschorf changed the title WIP: Refactor mesos scheduler initialization Refactor mesos scheduler initialization May 31, 2019

@pschorf pschorf requested review from scrosby and dposada May 31, 2019

Show resolved Hide resolved scheduler/config.edn Outdated
Show resolved Hide resolved scheduler/src/cook/components.clj
Show resolved Hide resolved scheduler/src/cook/compute_cluster.clj
(reset! current-driver driver)
(cc/set-mesos-driver-atom-hack! compute-cluster driver)

cluster-leadership-promise (cc/initialize-cluster compute-cluster

This comment has been minimized.

Copy link
@scrosby

scrosby Jun 7, 2019

Member

This is worth a comment. I remember you saying something about how you need to block a thread for the leader (or something like that.) Can you write here what you told me?

This comment has been minimized.

Copy link
@pschorf

pschorf Jun 11, 2019

Author Contributor

I added a comment below where I do the blocking read on the channel. Also renamed the variable.

This comment has been minimized.

Copy link
@scrosby

scrosby Jun 11, 2019

Member

Something like 'We leave a thread blocked .takeLeadership. When that thread is unblocked by curator, it will subsequently send a message on this channel, unblocking us here."

This comment has been minimized.

Copy link
@pschorf

pschorf Jun 11, 2019

Author Contributor

Did you see the comment below?

Show resolved Hide resolved scheduler/src/cook/mesos.clj Outdated
Show resolved Hide resolved scheduler/src/cook/mesos/mesos_compute_cluster.clj Outdated
(let [cluster-entity-id (get-mesos-cluster-entity-id (d/db conn) mesos-cluster)]
(when-not cluster-entity-id
(cc/write-compute-cluster conn (mesos-cluster->compute-cluster-map-for-datomic mesos-cluster)))
(create-mesos-compute-cluster compute-cluster-name

This comment has been minimized.

Copy link
@scrosby

scrosby Jun 7, 2019

Member

Why are we passing in a factory method here? If we have to, can we name it -factory or something?

This comment has been minimized.

Copy link
@pschorf

pschorf Jun 11, 2019

Author Contributor

That was how I got around the DI problem of needing some of the extra items from the component map to construct the new mesos compute cluster. Renamed.

This comment has been minimized.

Copy link
@scrosby

scrosby Jun 11, 2019

Member

Still don't like. :( But the least worst evil.

This comment has been minimized.

Copy link
@pschorf

pschorf Jun 11, 2019

Author Contributor

Yeah, hopefully some of this can be further cleaned up later. But I wanted to stick to moving things as much as possible in this PR and avoid changing too much.

Show resolved Hide resolved scheduler/src/cook/scheduler/scheduler.clj
Show resolved Hide resolved scheduler/test/cook/test/scheduler/scheduler.clj
Show resolved Hide resolved scheduler/src/cook/mesos/mesos_compute_cluster.clj Outdated

@pschorf pschorf force-pushed the pschorf:init-refactor branch from 7e0e053 to 116d508 Jun 11, 2019

@pschorf pschorf requested review from shamsimam and removed request for dposada Jun 12, 2019

Show resolved Hide resolved scheduler/src/cook/compute_cluster.clj Outdated
Show resolved Hide resolved scheduler/src/cook/components.clj
Show resolved Hide resolved scheduler/src/cook/compute_cluster.clj Outdated
Show resolved Hide resolved scheduler/src/cook/compute_cluster.clj Outdated
Show resolved Hide resolved scheduler/src/cook/mesos/mesos_compute_cluster.clj Outdated
Show resolved Hide resolved scheduler/src/cook/test/testutil.clj Outdated
Show resolved Hide resolved scheduler/src/cook/scheduler/scheduler.clj Outdated
@pschorf

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@shamsimam there was some nuance to that apply call, I made another commit to fix it.

@shamsimam shamsimam merged commit 6c6597f into twosigma:master Jun 12, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.