-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
pluggable scheduler for storm #224
Conversation
…nd executor.clj left
I have moved all the scheduler related methods to Cluster class, the intention is it will easier for scheduler developers, when he needs something, he just have a look at the Cluster api, now class like SupervisorDetails, TopologyDetails etc just contains static information. |
I have also written a demo scheduler here, to show the usage of Cluster api. https://github.com/xumingming/storm-lib/blob/master/src/jvm/storm/DemoScheduler.java |
…eaner, everything compiling, now need to get tests working
Found an issue: if a custom scheduler only schedule part of a topology, for example, a spout, the assignment for this spout might be reschedulered by our system scheduler(DefaultScheduler.clj) due to how DefaultScheduler currently works: DefaultScheduler will calculate the a distribution of the tasks in slots, if any slot does match the distribution, the slot will be freed, how do you think of this issue, Nathan? |
…tatuses in cluster mode)
dead-slots (if (nil? dead-slots) {} dead-slots) | ||
supervisor-infos (all-supervisor-info storm-cluster-state) | ||
supervisors (into {} (for [[sid supervisor-info] supervisor-infos | ||
:let [hostname (:hostname supervisor-info) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is actually wrong – this is not how available slots are determined. Available slots must be determined through the INimbus#availableSlots interface. This is what makes it possible to run Storm on other cluster resource managers like Mesos. In fact, standalone mode is considered to be just another resource manager which determines slots via Zookeeper registrations and the "meta" field provided by supervisors (See here: https://github.com/nathanmarz/storm/blob/master/src/clj/backtype/storm/daemon/nimbus.clj#L915 ).
To fix this, the availableSlots method on INimbus should be updated to have this signature:
Collection<WorkerSlot> availableSlots(Collection<SupervisorDetails> existingSupervisors, Collection<WorkerSlot> usedSlots, Collection<TopologyDetails> topology)
Then the standalone mode implementation should be updated as necessary (should be pretty straightforward).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not determine available slots here, what we need is indeed all slots, because even if a slot is used, custom scheduler code might want to free the slot and use it for some other topology.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xumingming That's right, all slots should be available slots + used slots.
@xumingming Added a couple comments into the pull request about a critical issue with this pull request. One more thing – 0.8.0's executor implementation redid a lot of the scheduling code, so there's going to be merge conflicts between this stuff and 0.8.0. If you want to update this pull request to be against 0.8.0, that would be appreciated, but if not I'll do it myself (it will be a pretty substantial merge). |
…need to fix some tests
@xumingming Can you change this pull request to be against the 0.8.0 branch of storm? |
Will send a new pull request to against branch 0.8.0, so close this one. |
for issue #164