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

Supervisor Resource capability configuration #64

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

roshanp
Copy link

@roshanp roshanp commented Jan 3, 2014

Includes:

  1. Ability to specify the supervisor resource (memory) through configuration, and finding the number of vcores based on the slots of the supervisor
  2. Tracking the number of supervisors already running, so that only one supervisor can run per host.
  3. Added a gitignore file to remove any files that should not be added to git

Notes:

  1. I tried to set up the ApplicationMasterTrackingUrl to be the Storm UI page, but I guess the yarn proxy cannot handle the CSS correctly. It does not render the page correctly, so I kept that out of the pull request
  2. I think that the best solution for the supervisor tracking would be to move to using the AMRMClientAsync. I had gone down this route but it required major modifications of code that probably wasn't necessary for what I wanted to do.
  3. I'm not sure how to write a test for this, any help would be appreciated.

Thanks

Punnoose, Roshan added 4 commits January 3, 2014 16:49
Keep track of the running supervisors within the StormAMRMClient. Make use of this list and the yarn blacklist feature to ensure that multiple supervisors do not allocate on the same node.
The supervisor resource size is now configurable through the storm-yarn configuration
@@ -63,7 +63,7 @@
</repositories>
<properties>
<storm.version>0.9.0-wip21</storm.version>
<hadoop.version>2.1.0-beta</hadoop.version>
<hadoop.version>2.2.0</hadoop.version>
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this requires Hadoop 2.2 then we probably want to find the correct version of CDH and HDP that also work with it and update them accordingly. (Just because people are going to ask for it)

Copy link
Author

Choose a reason for hiding this comment

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

Sure. HDP seems to be at 2.2.0.2.1.0.0-92.
CDH on the other hand seems to be on a SNAPSHOT? Wasn't sure: (https://repository.cloudera.com/artifactory/cloudera-repos/org/apache/hadoop/hadoop-common/)

@revans2
Copy link
Collaborator

revans2 commented Jan 3, 2014

I did a quick look through the code and for the most part it looks good. I had a few comments but nothing too terribly serious. Before I can merge anything in I am going to need you to read over https://github.com/yahoo/storm-yarn/wiki/Contributing and send a signed copy of the CLA to the email address listed there.

We need to come up with a much better integration testing for storm-yarn, probably something using the Hadoop mini cluster, but for now I would look into doing some unit tests. I kind of feel bad asking you to unit test code that doesn't have any real unit tests to start out with. If you could look at finding a good point to mock out the RM interactions that would be great. That might take moving to the AsyncClient for that. If it is too much of a pain to come up with if you could make sure you run some manual tests and show the result that would probably be enough.

@roshanp
Copy link
Author

roshanp commented Jan 3, 2014

Yeah I can do that, I'll try to get the CLA back to you soon. And get the modifications to the pull request in quickly.

As per mocking/unit testing, do you have any preferences? I usually like JMockit, but am open to anything.

@revans2
Copy link
Collaborator

revans2 commented Jan 6, 2014

For mocking storm currently uses Mockito 1.9.5. We hope to keep the door open so that this project can merge into storm proper at some point relatively soon. So if you could use that it would be great.

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